-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat(go/adbc/driver/snowflake): added table constraints implementation for GetObjects API #1593
feat(go/adbc/driver/snowflake): added table constraints implementation for GetObjects API #1593
Conversation
…for depth of table and columns. Also added tests.
|
csharp/test/Apache.Arrow.Adbc.Tests/Metadata/GetObjectsParser.cs
Outdated
Show resolved
Hide resolved
csharp/test/Apache.Arrow.Adbc.Tests/Metadata/GetObjectsParser.cs
Outdated
Show resolved
Hide resolved
csharp/test/Apache.Arrow.Adbc.Tests/Metadata/GetObjectsParser.cs
Outdated
Show resolved
Hide resolved
…ith a given name is not found
…urce handling for sql files
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.
LGTM once the license header is fixed up.
@zeroshade @lidavidm @CurtHagenlocher Could you please review the PR? |
Added the missing Apache License. Thanks James! |
import ( | ||
"cmp" | ||
"context" | ||
"database/sql" | ||
"database/sql/driver" | ||
"fmt" | ||
"io" | ||
"regexp" | ||
"slices" |
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.
the packages cmp
and slices
were only added in go1.21
so either we need to bump our minimum version or we need to replace the usages of these.
@lidavidm what do you think?
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'd be fine with bumping to 1.21 as our minimum version
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.
Looks like 1.20 is EOL. Could someone create a separate PR to bump?
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'll take care of that.
@ryan-syed after we merge the bump, I'll comment here so you can rebase.
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.
@zeroshade gentle reminder to check if the minimum version has been bumped up as this PR has been open for a while now.
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.
We ran into some issues with the packaging scripts when trying to bump the version. I'm looking into it with help from @kou
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 pushed a merge commit here.
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 beat me to it @lidavidm! lol
…ects_tableconstraints
Are the pre-commit changes strictly necessary here...? |
Alright, I'll try to fix up CI and merge this |
Issue
Table constraints implementation was missing for GetObjects
Fix
connection_test.go
for generated SQL statements.Design
Performance
After initial changes:
After additional changes to improve performance:
Before: