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.

Sign up for GitHub

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

Jump to bottom

refactor(oracle): port to sqlglot #8020

Merged
merged 68 commits into from
Jan 22, 2024
Merged

refactor(oracle): port to sqlglot #8020

merged 68 commits into from
Jan 22, 2024

Conversation

gforsyth
Copy link
Member

No description provided.

cpcloud
cpcloud requested changes Jan 19, 2024
View reviewed changes
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Generally speaking looks great 👌🏻

Just a few questions.

I will look into the impala/druid failures since they seem to be consistent (but are passing on upstream/the-epic-split)

ibis/backends/tests/test_client.py Show resolved Hide resolved
ibis/backends/tests/test_string.py Outdated Show resolved Hide resolved
ibis/backends/oracle/__init__.py Show resolved Hide resolved
ibis/backends/oracle/__init__.py
if isinstance(node, sg.exp.Table):
return sg.table(node.name, quoted=True)
elif isinstance(node, sg.exp.Column):
return sg.column(col=node.name, quoted=True)
Copy link
Member

Choose a reason for hiding this comment

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

Oooh, I hadn't really thought about this but I suspect we should probably apply this transform in general to backends whose quoted value is True.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, follow-up fodder.

ibis/backends/oracle/__init__.py
# artificially locked tables
cursor.close()
raise
df = OraclePandasData.convert_table(df, schema)
Copy link
Member

Choose a reason for hiding this comment

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

We have this method in many of the backends. Nothing to do here except note for a follow up to perhaps generalize a bit.

ibis/backends/oracle/compiler.py Outdated Show resolved Hide resolved
ibis/backends/oracle/compiler.py Outdated Show resolved Hide resolved
@cpcloud cpcloud mentioned this pull request Jan 19, 2024
Closed
21 tasks
gforsyth
gforsyth commented Jan 19, 2024
View reviewed changes
ibis/backends/oracle/compiler.py
Comment on lines +43 to +51
def _datatype_sql(self: Oracle.Generator, expression: sge.DataType) -> str:
# Use this to handle correctly formatting timestamp precision
# e.g. TIMESTAMP (scale) WITH TIME ZONE vs. TIMESTAMP WITH TIME ZONE(scale)
if expression.is_type("timestamptz"):
for exp in expression.expressions:
if isinstance(exp, sge.DataTypeParam):
return f"TIMESTAMP ({self.sql(exp, 'this')}) WITH TIME ZONE"
return "TIMESTAMP WITH TIME ZONE"
return self.datatype_sql(expression)
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if there's a better way to do this, but the regular mapping in Oracle.Generator.TYPE_MAPPING didn't expose the scale parameter

Copy link
Member

Choose a reason for hiding this comment

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

At some point we should upstream the bug report to sqlglot

Copy link
Member Author

Choose a reason for hiding this comment

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

tobymao/sqlglot#2879

@gforsyth
Copy link
Member Author

Ok, I have this down to 3 failing tests locally. I've been staring at it for long enough that I'm not going to get anywhere else today. I'll pick it back up on Monday.

@gforsyth gforsyth marked this pull request as ready for review January 19, 2024 20:32
gforsyth added 22 commits January 22, 2024 10:52
@gforsyth
test(oracle): fix up conftest for oracledb direct usage
346caa2
@gforsyth
refactor(oracle): add compiler and port backend
59cbfd1
@gforsyth
feat(oracle): add type mapper, converter, update compiler
fbc15d0
@gforsyth
feat(oracle): add rewrites for log2 and log10
73f7b78
@gforsyth
feat(oracle): cleanup
8a55ae7
@gforsyth
fix(oracle): infinities, degrees, radians, errors
875c4fe
@gforsyth
feat(oracle): port to sqlglot (numeric)
5e4bcbf
@gforsyth
feat(oracle): sqlglot strings
d6d2b1c
@gforsyth
feat(oracle): memtables
2d751b3
@gforsyth
fix(oracle): ensure column names are quoted
1da4715
@gforsyth
fix(oracle): smol stuff
86bcfc7
@gforsyth
fix(oracle): set up initial agg stuff
814ac79
@gforsyth
test(oracle): swap oracledb error for sqlalchemy error
e4a64cf
@gforsyth
fix(oracle): handle date with cast
26aaf87
@gforsyth
fix(oracle): handle sqlglot rewrites locally
91041a3
@gforsyth
fix(oracle): use group names, not numbers
bf9c527
@gforsyth
feat(oracle): count distinct, median, quantile
cb09a88
@gforsyth
feat(oracle): approx median
8d20054
@gforsyth
feat(oracle): countdistinct and countstar
b69e27e
@gforsyth
feat(oracle): use oracle positional bind parameters
91c5c85
@gforsyth
fix(oracle): add max size to varchar2 defs
50e991a
@gforsyth
fix(oracle): test fixups in asof join and numerics
e6635ba
cpcloud
cpcloud approved these changes Jan 22, 2024
View reviewed changes
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Assuming the tests eventually pass, this LGTM!

Epic!

@cpcloud cpcloud added oracle The Oracle backend tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split` tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main refactor Issues or PRs related to refactoring the codebase labels Jan 22, 2024
@gforsyth
fix(tests): filter None from tuples of raises
78cab6d 
In ibis/backends/tests/error.py we define the fallback exception types
as `None` if they cannot be imported (because the parent package isn't
installed).

This can lead to passing a tuple of possible exceptions that looks like

```
raises=(ImpalaHiveServer2Error, None, OracleDatabaseError)
```

If this gets passed without filteing, then you can end up with a gnarly
internal pytest error.

To avoid this, we filter out any `None` we find in `raises` while
handling the custom pytest markers.
@gforsyth
test(druid): remove xpassing xfail
70291fa
@gforsyth gforsyth merged commit 278c173 into ibis-project:the-epic-split Jan 22, 2024
49 checks passed
@gforsyth gforsyth deleted the oracle-sqlglot branch January 22, 2024 20:39
@cpcloud
Copy link
Member

cpcloud commented Jan 22, 2024

NICE

@gforsyth gforsyth mentioned this pull request Jan 23, 2024
Merged
@kszucs kszucs mentioned this pull request Feb 1, 2024
Closed
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
e7d3be9
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
6b5a354
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 1, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
0c5a405
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
a5eb1e8
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
3947559
kszucs pushed a commit to kszucs/ibis that referenced this pull request Feb 2, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (ibis-project#8020)
b2ec733
cpcloud pushed a commit to cpcloud/ibis that referenced this pull request Feb 4, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (ibis-project#8020)
8e520ec
cpcloud pushed a commit to cpcloud/ibis that referenced this pull request Feb 5, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (ibis-project#8020)
f5b1e6e
kszucs pushed a commit that referenced this pull request Feb 5, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (#8020)
d9cfdbc
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (#8020)
d403651
kszucs pushed a commit that referenced this pull request Feb 6, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (#8020)
1f0f3df
cpcloud pushed a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (ibis-project#8020)
1b8f815
cpcloud pushed a commit that referenced this pull request Feb 12, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (#8020)
884c1bd
cpcloud pushed a commit to cpcloud/ibis that referenced this pull request Feb 12, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (ibis-project#8020)
a76107e
cpcloud pushed a commit that referenced this pull request Feb 12, 2024
@gforsyth @cpcloud
refactor(oracle): port to sqlglot (#8020)
8be7753
kszucs pushed a commit that referenced this pull request Feb 12, 2024
@gforsyth @kszucs
refactor(oracle): port to sqlglot (#8020)
fbdc909
ncclementi pushed a commit to ncclementi/ibis that referenced this pull request Feb 21, 2024
@gforsyth @ncclementi
refactor(oracle): port to sqlglot (ibis-project#8020)
a40524e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpcloud cpcloud cpcloud approved these changes

Assignees
No one assigned
Labels
oracle The Oracle backend refactor Issues or PRs related to refactoring the codebase tes-required-for-merge Issues that must addressed before merging the-epic-split branch into main tes-required-for-release Things that must be addressed before *release* of `main` after merging in `the-epic-split`
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gforsyth @cpcloud