Oracle support#2349
Merged
Merged
Conversation
Closed
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…adata/soda-core into obsl-591-core-add-oracledb-support2
for more information, see https://pre-commit.ci
…data/soda-core into obsl-591-core-add-oracledb-support2
…-sql-ast' into obsl-591-core-add-oracledb-support2
…-sql-ast' into obsl-591-core-add-oracledb-support2
…data/soda-core into obsl-591-core-add-oracledb-support2
for more information, see https://pre-commit.ci
…data/soda-core into obsl-591-core-add-oracledb-support2
paulteehan
marked this pull request as ready for review
August 4, 2025 10:10
mivds
reviewed
Aug 4, 2025
mivds
left a comment
mivds
left a comment
Contributor
There was a problem hiding this comment.
Looks pretty good! Noticed a few odd quirks though; I left a few questions/suggestions
Comment on lines
+67
to
+72
| id | ||
| for id in [ # prefixes can be None if information schema is top-level, e.g. Oracle | ||
| *prefixes, | ||
| self.sql_dialect.schema_information_schema(), | ||
| ] | ||
| if id |
Contributor
There was a problem hiding this comment.
Hmm, wondering if we can't resolve this by passing prefixes=[] to MetadataColumnsQuery.__init__? Then we use the already available interface instead of adding hidden logic here
| self.sql_dialect.column_table_name(), | ||
| col | ||
| for col in [ | ||
| self.sql_dialect.column_table_catalog(), # this could be None for some data sources e.g. Oracle |
Contributor
There was a problem hiding this comment.
Is it an option to have this function return "NULL", i.e. SELECT NULL, schema_name, table_name FROM ...? That way we don't need to add conditionals here or in the result processing.
Comment on lines
+8
to
9
| # Attention, if dt does not have timezone info, Python will assume dt is in the local system timezone (e.g. UTC+1) | ||
| return dt.astimezone(timezone.utc).isoformat(timespec="seconds") |
Contributor
There was a problem hiding this comment.
Nice find with that comment. Out of scope for this PR, but I suspect this may not be the behavior we want.
Probably want to do something like this instead:
if dt.tzinfo is None:
dt = dt.replace(tzinfo=timezone.utc)
return dt.isoformat(timespec="seconds")That would interpret the timestamp as UTC if no timezone is provided (which is more reproducible than using the local timezone). Then formats to string including timezone, which may be UTC or a previously set timezone
Contributor
There was a problem hiding this comment.
Very big fan of this! Ran into issues with the "auto-conversion" that happens here.
With all the timezone stuff we're doing for this PR and for Oracle, I think we can definitely change it (if it doesn't break other things!!)
Comment on lines
+626
to
+627
| def sql_expr_timestamp_with_tz_literal(self, datetime_in_iso8601: str) -> str: | ||
| return self.sql_expr_timestamp_literal(datetime_in_iso8601) |
Contributor
There was a problem hiding this comment.
Hmm, not sure what the best approach is here. I fear this function as-is will be hard to use because you will always need to be aware of whether the datetime you have contains a timezone or not, and call the appropriate function. We could have a single function that detects if timezone is present and then acts accordingly. On the other hand that may lead to bugs because it works differently than expected.
I guess let's keep it as you've implemented it for now. We'll find out later on if there are places where we don't know which function to call.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…data/soda-core into obsl-591-core-add-oracledb-support2
for more information, see https://pre-commit.ci
|
nielsn
pushed a commit
that referenced
this pull request
Aug 8, 2025
* Initial oracle support * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * Added create_table to sql_ast * Added insert into statements to sql_ast * Added newlines to new statements so it's more readable * Updated some sql ast stuff based on needs from soda-extensions * Added drop table to the AST * Add support for Oracle * Code style updates * Add support for Oracle * Add support for Oracle * Allow insert into using select statements * Allow insert into using select statements * Additional fixes to support Oracle * Additional fixes to support Oracle * Revert accidental changes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix test * fix package version * Fix date formatting in sqlserver * Fix date formatting in sqlserver * Parse other type of connect string * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove oracle * Clarify prefix joining * Clarify prefix joining * Refactor metadata queries * Fix review comments * Add comment * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add timestamp and timestamp_tz insert tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Test timestamp insert * Extend test * cleanup * Fix failing tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Niels Bylois <niels.bylois@soda.io>
nielsn
added a commit
that referenced
this pull request
Aug 8, 2025
* Cleanning up the DWH tables to add the failed scan rows table * Sql dialect extensions for diagnostic warehouse * Added sql dialect max length * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * - Align naming for `STAR` alias - Remove `IN` for `STAR` because confusing - Some TODO's for typing * Align semicolon treatment in `create_schema_if_not_exists_sql` with rest of codebase * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Oracle support, AST enhancements, timezone tests (#2349) * Initial oracle support * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fixes to and extensions to support OracleDB * Added create_table to sql_ast * Added insert into statements to sql_ast * Added newlines to new statements so it's more readable * Updated some sql ast stuff based on needs from soda-extensions * Added drop table to the AST * Add support for Oracle * Code style updates * Add support for Oracle * Add support for Oracle * Allow insert into using select statements * Allow insert into using select statements * Additional fixes to support Oracle * Additional fixes to support Oracle * Revert accidental changes * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Fix test * fix package version * Fix date formatting in sqlserver * Fix date formatting in sqlserver * Parse other type of connect string * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Remove oracle * Clarify prefix joining * Clarify prefix joining * Refactor metadata queries * Fix review comments * Add comment * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Add timestamp and timestamp_tz insert tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Test timestamp insert * Extend test * cleanup * Fix failing tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Niels Bylois <niels.bylois@soda.io> * Add new plugin stubs for request proposals (#2361) * Add new plugin stubs for request proposals * Add create schema support for DuckDB --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Niels Nuyttens <niels@soda.io> Co-authored-by: paulteehan <45394252+paulteehan@users.noreply.github.com> Co-authored-by: Niels Bylois <niels.bylois@soda.io> Co-authored-by: Lauren De bruyn <lauren@soda.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Oracle implementation in soda-core. Tests passing locally, I think CI may not be configured yet. Ready for review.
Still pending: