Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor(csharp/test/Drivers/Interop/Snowflake): Updated the metadata tests to work without the db name #1352

Merged
merged 14 commits into from
Jan 11, 2024
Merged

refactor(csharp/test/Drivers/Interop/Snowflake): Updated the metadata tests to work without the db name #1352

merged 14 commits into from
Jan 11, 2024

Conversation

ryan-syed
Copy link
Contributor

@ryan-syed ryan-syed commented Dec 14, 2023 •
edited
Loading

Changes:

  • Modified tests to check the table type returned in DriverTests.cs
  • The database and schema name is not required for the getting the metadata anymore. The metadata calls in the Driver prefix the specific DB name whose INFORMATION_SCHEMA tables need to be accessed.

@github-actions github-actions bot added this to the ADBC Libraries 0.9.0 milestone Dec 14, 2023
@ryan-syed ryan-syed mentioned this pull request Dec 14, 2023
Closed
@ryan-syed
Copy link
Contributor Author

Similar to PR: #1351, except for populateMetadata, prepareDbSchemasSQL, prepareTablesSQL, and prepareColumnsSQL.

The implementation is based on proposal: https://github.com/apache/arrow-adbc/issues/1332##Proposal and improving existing API implementation.

@ryan-syed ryan-syed mentioned this pull request Dec 14, 2023
Closed
@ryan-syed ryan-syed changed the title fix: Reduced SQL calls in GetObjects to two, added prefixing DbName f… Reduced SQL calls in GetObjects to two, added prefixing DbName f… Dec 14, 2023
@github-actions GitHub Actions
Copy link

⚠️ Please follow the Conventional Commits format in CONTRIBUTING.md for PR titles.

@ryan-syed ryan-syed changed the title Reduced SQL calls in GetObjects to two, added prefixing DbName f… go/adbc/driver/snowflake: Reduced SQL calls in GetObjects to two, added prefixing DbName for INFORMATION_SCHEMA Dec 14, 2023
@ryan-syed ryan-syed marked this pull request as ready for review December 15, 2023 02:05
@ryan-syed ryan-syed changed the title go/adbc/driver/snowflake: Reduced SQL calls in GetObjects to two, added prefixing DbName for INFORMATION_SCHEMA perf(go/adbc/driver/snowflake): Reduced SQL calls in GetObjects to two, added prefixing DbName for INFORMATION_SCHEMA Dec 15, 2023
@lidavidm lidavidm removed this from the ADBC Libraries 0.9.0 milestone Dec 19, 2023
@AnithaPanduranganMS AnithaPanduranganMS mentioned this pull request Dec 21, 2023
Merged
@ryan-syed
fix: Reduced SQL calls in GetObjects to two, added prefixing DbName f…
b76af78 
…or INFORMATION_SCHEMA calls, and removed SQL cursor code

Additional changes:
* Reduced SQL calls by making only 1 - 2 SQL call based on the ObjectsDepth and using that data to populate all the previous depth information
* There is one SHOW TERSE DATABASES that is always called and the catalogPattern is used to filter the databases and prepare the SQL based on the depth for schema, tables, or columns
* The SQL cursor code was removed and the replaced with a static SQL that is prepared in Go based on the databases that match the catalogPattern
* GetObjects populates the MetadataRecords by making the necessary SQL call based on ObjectsDepth
* Modified the logic of GetObjects Init to pass MetadataRecords in getObjectsDbSchemas and getObjectsTables
* Modified tests to check the table type returned
@ryan-syed
refactor: removed unused code
12b9791
@ryan-syed
refactor: Refactored the SQL calls
de548d7
@ryan-syed
refactor: Refactored SQL, made the code modular, and added tests for …
f274627 
…verifying the generated SQL
@ryan-syed
refactor: Refactored all the filter conditions for GetObjects into a …
564012b 
…single prepareFilterConditions function
@github-actions github-actions bot added this to the ADBC Libraries 0.9.0 milestone Jan 2, 2024
@ryan-syed ryan-syed force-pushed the dev/ryansyed/fix_getobjects_withoutdbname_reducingsqlcalls_nocursor branch from b5e148c to b76af78 Compare January 2, 2024 23:00
@ryan-syed
Copy link
Contributor Author

Functionality changes seems to be merged already in PR: #1338. Some trivial changes for DriverTests.cs are pending.

@ryan-syed ryan-syed requested a review from jduo January 5, 2024 20:51
@CurtHagenlocher
Copy link
Contributor

So the only material changes remaining are in the test?

@lidavidm
Copy link
Member

lidavidm commented Jan 9, 2024

If so do you mind reverting the spurious Go changes and updating the PR title and description?

@ryan-syed ryan-syed changed the title perf(go/adbc/driver/snowflake): Reduced SQL calls in GetObjects to two, added prefixing DbName for INFORMATION_SCHEMA refactor(csharp/test/Drivers/Interop/Snowflake): Updated the metadata tests to work without the db name Jan 11, 2024
@ryan-syed
Copy link
Contributor Author

If so do you mind reverting the spurious Go changes and updating the PR title and description?

I have removed the whitespace changes and updated the title and description.

@CurtHagenlocher CurtHagenlocher merged commit abb758c into apache:main Jan 11, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhcoe davidhcoe davidhcoe left review comments

@CurtHagenlocher CurtHagenlocher CurtHagenlocher approved these changes

@zeroshade zeroshade Awaiting requested review from zeroshade

@lidavidm lidavidm Awaiting requested review from lidavidm lidavidm is a code owner

@kou kou Awaiting requested review from kou

@paleolimbot paleolimbot Awaiting requested review from paleolimbot

@jduo jduo Awaiting requested review from jduo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
ADBC Libraries 0.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ryan-syed @CurtHagenlocher @lidavidm @jduo @davidhcoe