-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
…umn [run CI]
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
=======================================
Coverage ? 77.84%
=======================================
Files ? 160
Lines ? 62064
Branches ? 8769
=======================================
Hits ? 48312
Misses ? 11625
Partials ? 2127 |
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! Thanks @IsaacWarren
} | ||
) | ||
|
||
query = 'SELECT * FROM "read_namespace"."bodo_iceberg_read_test"' |
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.
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> |
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.
Can you check how these changes impact the package size? May be relevant for the next pip publication.
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 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. |
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.
[sic] Glue
* @param streamingOptions The options to use if streaming is enabled. | ||
* @return The generated code to produce a read. | ||
*/ | ||
override fun generateReadCode( |
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.
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.
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.
Good idea, most of this is identical to glue/tabular so we could have some kind of "normal" iceberg metclass, basically everything but snowflake
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, Thanks @IsaacWarren !
...odosql-calcite-application/src/main/java/com/bodosql/calcite/application/PythonEntryPoint.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: scott-routledge2 <scott@bodo.ai>
Customer test issues are unrelated, will fix in another PR, merging |
Co-authored-by: Nicholas <njriasanovsky@berkeley.edu> Co-authored-by: scott-routledge2 <scott@bodo.ai>
Changes included in this PR
Add S3 Tables support for bodosql
Testing strategy
PR CI
User facing changes
BodoSQL support for s3 tables
Checklist
[run CI]
in your commit message.