-
Notifications
You must be signed in to change notification settings - Fork 35
Add support for Vector index syntax in T-SQL parser with separate index option classes #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… generators Co-authored-by: llali <17187348+llali@users.noreply.github.com>
| internal const string Json = "JSON"; | ||
| internal const string JsonArray = "JSON_ARRAY"; | ||
| internal const string JsonObject = "JSON_OBJECT"; | ||
| internal const string Vector = "VECTOR"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this class already has a Vector filed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 97a1590. Removed the duplicate Vector constant at line 517, keeping the existing one at line 1073.
| <Member Name="Expression" Type="ScalarExpression" Summary="Can be an integer in the range 0 - 10080."/> | ||
| <Member Name="TimeUnit" Type="CompressionDelayTimeUnit" GenerateUpdatePositionInfoCall="false" Summary="The optional unit of measure for the compression delay (it can be MINUTE or MINUTES)."/> | ||
| </Class> | ||
| <Class Name="VectorMetricIndexOption" Base="IndexOption" Summary="Represents the vector metric index option."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create one VectorIndexOption class that includes both MetricType and VectorType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 97a1590. Consolidated VectorMetricIndexOption and VectorTypeIndexOption into a single VectorIndexOption class that includes both MetricType and VectorType properties, along with updated grammar and script generator.
Co-authored-by: llali <17187348+llali@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
SqlScriptDom/Parser/TSql/Ast.xml
Outdated
| <Member Name="Expression" Type="ScalarExpression" Summary="Can be an integer in the range 0 - 10080."/> | ||
| <Member Name="TimeUnit" Type="CompressionDelayTimeUnit" GenerateUpdatePositionInfoCall="false" Summary="The optional unit of measure for the compression delay (it can be MINUTE or MINUTES)."/> | ||
| </Class> | ||
| <Class Name="VectorIndexOption" Base="IndexOption" Summary="Represents the vector index option that includes both metric type and vector type."> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a mistake. you were right about creating two Index Options one for MetricType and one for VectorType. please revert back to your original change with two IndexOption types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted to original design in 0c53470. The implementation now uses two separate classes:
VectorMetricIndexOptionfor METRIC optionsVectorTypeIndexOptionfor TYPE options
All functionality is preserved while following the original architecture.
…ectorTypeIndexOption classes Co-authored-by: llali <17187348+llali@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| ON MySchema.MyTable (VectorColumn) WITH (METRIC = 'euclidean'); | ||
|
|
||
| CREATE VECTOR INDEX [IX Vector Index] | ||
| ON [dbo].[Documents] ([Vector Data]) WITH (METRIC = 'cosine'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change option values like cosine dot to uppercase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated all vector index option values to uppercase in the baseline file. Changed 'cosine' → 'COSINE', 'dot' → 'DOT', 'euclidean' → 'EUCLIDEAN', and 'DiskANN' → 'DISKANN' as requested. Commit df44c72.
Co-authored-by: llali <17187348+llali@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
zijchen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some error test cases to ParserErrorsTests
Co-authored-by: zijchen <13544267+zijchen@users.noreply.github.com>
Added comprehensive error test cases for vector index syntax in e0432d2. The new Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Test/SqlDom/ParserErrorsTests.cs
Outdated
| public void VectorIndexNegativeTests() | ||
| { | ||
| // Missing VECTOR keyword | ||
| ParserTestUtils.ErrorTest170("CREATE INDEX IX_Test ON dbo.Documents (VectorData)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this valid syntax for normal index creation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think the CI tests are failing because of this, you can just remove this test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! The statement CREATE INDEX IX_Test ON dbo.Documents (VectorData) is valid syntax for creating a regular index, not an error case. Removed this invalid test case in 849df29.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're absolutely right! I've already removed that test case in commit 849df29. The CREATE INDEX IX_Test ON dbo.Documents (VectorData) statement is indeed valid syntax for creating a regular index and shouldn't be in the vector index error tests.
…tax is valid for regular indexes Co-authored-by: zijchen <13544267+zijchen@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This PR adds comprehensive support for parsing and generating T-SQL VECTOR index statements in SqlScriptDOM with separate, dedicated classes for vector index options.
Syntax Support
The implementation supports the full vector index syntax specification:
Architecture
Dedicated Index Option Classes:
VectorMetricIndexOptionclass handles METRIC options withMetricTypepropertyVectorTypeIndexOptionclass handles TYPE options withVectorTypepropertyIndexOptionKind.VectorMetricandIndexOptionKind.VectorTypefor clear distinctionParser Support:
createVectorIndexStatementgrammar rule following JSON index patternvectorMetricOptionandvectorTypeOptionrules for each option typeScript Generation:
VectorMetricIndexOptionandVectorTypeIndexOptionvisitorsTest Coverage:
The implementation adds 315 lines of code with no deletions, following established patterns for maintainability and using separate classes for each vector index option type.
Fixes #149.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.