-
Notifications
You must be signed in to change notification settings - Fork 14k
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
chore(db_engine_specs): Refactor get_index #23656
chore(db_engine_specs): Refactor get_index #23656
Conversation
df = database.get_df(sql, schema) | ||
return column_names, cls._latest_partition_from_df(df) | ||
|
||
return column_names, cls._latest_partition_from_df( |
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.
Same logic as before just a cleaner presentation including adding keyword arguments, i.e., previously it wasn't apparent in cls._partition_query(table_name, database, 1, part_fields)
what 1
and part_fields
meant.
3d77d2a
to
bb68e0f
Compare
f9868f3
to
d631e64
Compare
Codecov Report
@@ Coverage Diff @@
## master #23656 +/- ##
=======================================
Coverage 68.08% 68.08%
=======================================
Files 1920 1920
Lines 73984 73990 +6
Branches 8092 8092
=======================================
+ Hits 50374 50379 +5
- Misses 21539 21540 +1
Partials 2071 2071
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
780e135
to
fd1f5cd
Compare
fd1f5cd
to
ebd79e5
Compare
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
table_name, | ||
database, |
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.
table_name, | |
database, | |
table_name=table_name, | |
database=database, |
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 tend to only provide keywords if the variable names differ.
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.
No problem. I generally like to always provide the pair to remove any ordering requirements. By the way, keyword arguments is one of my favorite Python features.
(cherry picked from commit b35b5a6)
(cherry picked from commit b35b5a6)
(cherry picked from commit b35b5a6)
SUMMARY
Rather than having a
normalize_indexes
method for normalizing the response of the DB-APIget_indexes
function, the DB engine spec should provide a more flexible utility method namedget_indexes
—akin toget_table_names
,get_column_names
, etc. with access to the database et al. objects and can be overridden.The motivation for the change is at Airbnb for Trino we have both Iceberg and Hive backed tables where the Trino DB-API
get_indexes
method returns an empty list for Iceberg backed tables. Providing aget_indexes
method within the DB engine spec allows us to easily add custom logic to handle both scenarios within our derived engine spec.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI. Added unit tests.
ADDITIONAL INFORMATION