-
Notifications
You must be signed in to change notification settings - Fork 109
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: Support BQ decimal precision and scale for schema validation #960
Conversation
nehanene15
commented
Aug 28, 2023
- Limit Teradata query to one row for custom query schema generation (Avoids full table scan)
- Support non-defualt BigQuery precision/scale (Closes Schema validation: BigQuery Decimal Precision #883 and Closes Schema Validation : Bigquery Numeric #882 )
…imit Teradata query to one row for custom query schema
/gcbrun |
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 aside from comment about Hive (and maybe other engines) having the full scan issue fixed here for Teradata.
@@ -141,7 +141,7 @@ def get_schema(self, table_name: str, database: str = None) -> sch.Schema: | |||
return sch.Schema(schema) | |||
|
|||
def _get_schema_using_query(self, query): | |||
cur = self.raw_sql(query) | |||
cur = self.raw_sql(f"SELECT TOP 1 * FROM ({query}) AS t0") |
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 might also have this problem for other engines, e.g: Hive:
def _get_schema_using_query(self, query):
# Removing LIMIT 0 around query since it returns no results in Hive
cur = self.raw_sql(query)
cur.fetchall()
ibis_fields = self._adapt_types(cur.description)
cur.release()
return sch.Schema(ibis_fields)
We should be able to use fetchone
in place of fetchall
for this. Or embed in a subquery. I didn't check all engines so Hive may not be the only one.
Happy for this to be spun into another issue if you need to move quickly on this one.
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 catch! I'll update this for Hive and add a LIMIT 1
similar to the Ibis impala connection. Spanner only materializes one row, and all other DBs are handled by Ibis.