[Do not merge] : [@azure/cosmos] Add Full text search support for cosmos-explorer #31710

amanrao23 · 2024-11-11T06:29:38Z

Packages impacted by this PR

@azure/cosmos

Issues associated with this PR

Describe the problem that is addressed by this PR

This PR adds support for FTS for cosmosdb-explorer

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Yes

Provide a list of related PRs (if any)

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

Checklists

Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

### Packages impacted by this PR ### Issues associated with this PR ### Describe the problem that is addressed by this PR ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [ ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Manik Khandelwal <[email protected]>

### Packages impacted by this PR ### Issues associated with this PR ### Describe the problem that is addressed by this PR ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [ ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Ujjwal Soni <[email protected]>

### Packages impacted by this PR ### Issues associated with this PR ### Describe the problem that is addressed by this PR ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [ ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Manik Khandelwal <[email protected]>

### Packages impacted by this PR ### Issues associated with this PR ### Describe the problem that is addressed by this PR ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? ### Are there test cases added in this PR? _(If not, why?)_ ### Provide a list of related PRs _(if any)_ ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [ ] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [ ] Added a changelog (if necessary) --------- Co-authored-by: Manik Khandelwal <[email protected]> Co-authored-by: Ujjwal Soni <[email protected]>

sdk/cosmosdb/cosmos/CHANGELOG.md

topshot99 · 2024-11-11T12:10:59Z

sdk/cosmosdb/cosmos/src/queryExecutionContext/ExecutionContext.ts

+  operationOptions?: QueryOperationOptions;
+  ruConsumed?: RUConsumedManager;
+}
+export interface ExecutionContextHybridOptions extends ExecutionContextOptions {


we don't need it now, can be removed.

sdk/cosmosdb/cosmos/src/queryExecutionContext/defaultQueryExecutionContext.ts

topshot99 · 2024-11-11T15:40:23Z

sdk/cosmosdb/cosmos/test/public/functional/NonStreamingQueryPolicy.spec.ts

@@ -333,7 +325,7 @@ describe("Vector search feature", async () => {
    });
  });

-  describe.skip("Vector Search Query large dataset", async () => {
+  describe("Vector Search Query large dataset", async () => {


@amanrao23 we have purposefully skipped it. We don't want to execute large queries over our pipeline, this might impact our pipeline time.

maorleger

Given the urgency I am going to approve with comments - but please do take my concerns into consideration, it can be in a follow-up PR post your deadline🙏

maorleger · 2024-11-13T17:14:49Z

sdk/cosmosdb/cosmos/review/cosmos.api.md

@@ -1095,6 +1097,23 @@ export class FeedResponse<TResource> {
    readonly resources: TResource[];
 }

+// @public
+export interface FullTextIndex {


FullTextSearchIndex, FullTextSearchPath, FullTextSearchPolicy might be clearer, just fullText doesn't tell me much

maorleger · 2024-11-13T17:15:58Z

sdk/cosmosdb/cosmos/review/cosmos.api.md

@@ -1764,6 +1794,11 @@ export const QueryMetricsConstants: {
    SchedulingMetricsText: string;
 };

+// @public (undocumented)
+export interface QueryOperationOptions {
+    ruCapPerOperation?: number;


What does ru mean? Can we find a clearer name?

It's a well known concept for request units in the case of Cosmos DB https://learn.microsoft.com/en-us/azure/cosmos-db/request-units

maorleger · 2024-11-13T17:16:31Z

sdk/cosmosdb/cosmos/review/cosmos.api.md

@@ -2004,6 +2039,15 @@ export interface RetryOptions {
    maxWaitTimeInSeconds: number;
 }

+// @public (undocumented)


I noticed a bunch of shipped and new undocumented code - is that expected?

maorleger · 2024-11-13T17:17:46Z

sdk/cosmosdb/cosmos/src/common/RUConsumedManager.ts

+
+  addToRUConsumed(value: number): Promise<void> {
+    return new Promise((resolve) => {
+      this.semaphore.take(() => {


Why is a semaphore needed for synchronous operations? I thought JavaScript has run-to-completion semantics but maybe I missed something?

maorleger · 2024-11-13T17:19:13Z

sdk/cosmosdb/cosmos/test/public/integration/text-3properties-1536dimensions-100documents.ts

Instead of committing a large file like this, can you generate it on-the-fly or as part of test resources?

maorleger · 2024-11-13T17:20:16Z

sdk/cosmosdb/cosmos/src/utils/envUtils.ts

@@ -4,7 +4,5 @@
 import process from "node:process";
 import { Constants } from "../common/constants";

-export const disableListAndSetAggregate: boolean =
-  process.env.DISABLE_LIST_AND_SET_AGGREGATE === "true";


What about "True", "1", "enabled", etc? Or do we document that only "true" is accepted?

github-actions · 2025-01-17T05:03:36Z

Hi @amanrao23. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

topshot99 and others added 18 commits October 30, 2024 17:50

Add RU cap

96647b5

fix fts+ruCap

b859b13

Remove disableMakeSetMakeList option

d4799a6

Update executionContexts options

16c4ca4

Enable tests

26d0929

amanrao23 requested review from bterlson, a team, aditishree1, sajeetharan and topshot99 as code owners November 11, 2024 06:29

github-actions bot added the Cosmos label Nov 11, 2024

amanrao23 added 6 commits November 11, 2024 12:05

Update changelog

9c8cd6d

Update formatting

6a00339

fix lint

3349ef2

fix lint

82b2974

skip tests for pipeline

f348d4d

fix ru cap check

00f5aef

amanrao23 added 2 commits November 11, 2024 14:54

fix format

7ae8287

fix package name

fa097e0

topshot99 reviewed Nov 11, 2024

View reviewed changes

sdk/cosmosdb/cosmos/CHANGELOG.md Outdated Show resolved Hide resolved

Update ChangeLog

af72225

topshot99 reviewed Nov 11, 2024

View reviewed changes

sdk/cosmosdb/cosmos/src/queryExecutionContext/defaultQueryExecutionContext.ts Show resolved Hide resolved

amanrao23 added 2 commits November 11, 2024 18:21

update executionContext options interface

e46c167

Fix formatting

28901d1

topshot99 reviewed Nov 11, 2024

View reviewed changes

amanrao23 added 4 commits November 13, 2024 19:05

Fix FetchAll returning empty result

6457b0e

Cherry-pick from fts

bd40fe2

skip fts emulator tests

c38f6d6

fix fetchAll

a9fc970

maorleger approved these changes Nov 13, 2024

View reviewed changes

amanrao23 added 3 commits November 15, 2024 02:19

fix for gw placeholder issue

02f24b8

Update Changelog

fa230a2

Update Changelog

4bb6efa

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label Jan 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Do not merge] : [@azure/cosmos] Add Full text search support for cosmos-explorer #31710

[Do not merge] : [@azure/cosmos] Add Full text search support for cosmos-explorer #31710

amanrao23 commented Nov 11, 2024

topshot99 Nov 11, 2024

topshot99 Nov 11, 2024

maorleger left a comment

maorleger Nov 13, 2024

maorleger Nov 13, 2024

mpodwysocki Nov 13, 2024

maorleger Nov 13, 2024

maorleger Nov 13, 2024

maorleger Nov 13, 2024

maorleger Nov 13, 2024

github-actions bot commented Jan 17, 2025

[Do not merge] : [@azure/cosmos] Add Full text search support for cosmos-explorer #31710

Are you sure you want to change the base?

[Do not merge] : [@azure/cosmos] Add Full text search support for cosmos-explorer #31710

Conversation

amanrao23 commented Nov 11, 2024

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2025

Command used to generate this PR:***(Applicable only to SDK release request PRs)*