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.

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

BSE-4358: BodoSQL support for S3 tables #126

Merged
merged 81 commits into from
Jan 10, 2025
Merged

Conversation

IsaacWarren
Copy link
Contributor

Changes included in this PR

Add S3 Tables support for bodosql

Testing strategy

PR CI

User facing changes

BodoSQL support for s3 tables

Checklist

  • Pipelines passed before requesting review. To run CI you must include [run CI] in your commit message.
  • I am familiar with the Contributing Guide
  • I have installed + ran pre-commit hooks.

Sorry, something went wrong.

…umn [run CI]

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren
Base automatically changed from nick/remove_remaining_java to main January 7, 2025 22:25
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@4961641). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #126   +/-   ##
=======================================
  Coverage        ?   77.84%           
=======================================
  Files           ?      160           
  Lines           ?    62064           
  Branches        ?     8769           
=======================================
  Hits            ?    48312           
  Misses          ?    11625           
  Partials        ?     2127           

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren
@IsaacWarren IsaacWarren marked this pull request as ready for review January 8, 2025 15:08
Copy link
Contributor

@njriasan njriasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @IsaacWarren

}
)

query = 'SELECT * FROM "read_namespace"."bodo_iceberg_read_test"'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to list a docstring for what must exist and where at the top. In case we ever do an account cleanup/deletion. Also if there are creation steps including those would be great.

@@ -33,7 +33,7 @@
<!-- Start Iceberg Dependencies. We may remove these as we cleanup the relationship with the Iceberg Connector. -->
<iceberg.version>1.5.2</iceberg.version>
<hadoop.version>3.3.3</hadoop.version>
<aws.version>2.20.26</aws.version>
<aws.version>2.29.26</aws.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check how these changes impact the package size? May be relevant for the next pip publication.

Copy link
Contributor Author

@IsaacWarren IsaacWarren Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wheel is 3MB bigger since the last release, no cause for concern


/**
* Return the number of levels at which a default schema may be found.
* Glue catalogs don't have a default schema so always returns 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sic] Glue

* @param streamingOptions The options to use if streaming is enabled.
* @return The generated code to produce a read.
*/
override fun generateReadCode(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have to add more of these it may be useful to have an abstract parent class that defines the limited behavior for certain catalogs. For example if append and remote queries don't work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, most of this is identical to glue/tabular so we could have some kind of "normal" iceberg metclass, basically everything but snowflake

Copy link
Contributor

@scott-routledge2 scott-routledge2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks @IsaacWarren !

IsaacWarren and others added 5 commits January 9, 2025 14:55

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: scott-routledge2 <scott@bodo.ai>
Fix doc

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: scott-routledge2 <scott@bodo.ai>

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren

Verified

This commit was signed with the committer’s verified signature.
IsaacWarren Isaac Warren
@IsaacWarren
Copy link
Contributor Author

Customer test issues are unrelated, will fix in another PR, merging

@IsaacWarren IsaacWarren merged commit 3fb4444 into main Jan 10, 2025
21 of 23 checks passed
@IsaacWarren IsaacWarren deleted the isaac/bodosql_s3_tables branch January 10, 2025 14:25
DrTodd13 pushed a commit that referenced this pull request Jan 21, 2025
Co-authored-by: Nicholas <njriasanovsky@berkeley.edu>
Co-authored-by: scott-routledge2 <scott@bodo.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants