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.

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(datafusion, flink, mssql): add uuid operation #8545

Merged
merged 5 commits into from
Mar 7, 2024

Conversation

jitingxu1
Copy link
Contributor

@jitingxu1 jitingxu1 commented Mar 5, 2024

Description of changes

Continued from PR:
add uuid operation for mssql, flink, datafusion,

Issues closed

@@ -227,6 +227,14 @@ def visit_RandomScalar(self, op):
# Not using FuncGen here because of dotted function call
return sg.func("dbms_random.value")

def visit_RandomUUID(self, op):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcrist There is no builtin uuid in oracle, I used the sys_guid and reformat it into a uuid format, but not sure if it is ok. since it a not a real uuid, it fails the version == 4 test.

Copy link
Member

Choose a reason for hiding this comment

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

If there's no uuid, then there's no uuid -- this can remain unimplemented for the Oracle backend unless we are very confident that the semantics match other implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove oracle for now.

@@ -70,6 +70,7 @@ class PySparkCompiler(SQLGlotCompiler):
ops.Hash: "hash",
ops.Log10: "log10",
ops.LStrip: "ltrim",
ops.RandomUUID: "uuid",
Copy link
Contributor Author

@jitingxu1 jitingxu1 Mar 5, 2024

Choose a reason for hiding this comment

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

@jcrist There was a bug in the previous version of Spark, It make our unit test failed. it has been addressed in the latest 4.0 version.(spark jira issue and pr).

Copy link
Member

Choose a reason for hiding this comment

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

We don't currently support spark 4.0, so I don't think we should provide this mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we open an issue documenting this so when we support spark 4.0 (not sure how realistic that is) then this could be a nice onboarding issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove pyspark uuid support for now and open an issue for future reference.

@jitingxu1 jitingxu1 requested a review from jcrist March 5, 2024 08:01
@jitingxu1
Copy link
Contributor Author

@gforsyth and @ncclementi I removed the oracle and pyspark, please take a review again. Thanks for your time.

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

Thanks @jitingxu1 ! The code here looks good.

Can you update the PR title to match what's implemented here? Something like:

feat(datafusion, flink, mssql): add uuid operation

And then update the PR description to reflect the current code state? That's what end up as the body of the commit on main.

@jitingxu1 jitingxu1 changed the title feat: add uuid feat(datafusion, flink, mssql): add uuid operation Mar 7, 2024
@jitingxu1
Copy link
Contributor Author

Thanks @jitingxu1 ! The code here looks good.

Can you update the PR title to match what's implemented here? Something like:

feat(datafusion, flink, mssql): add uuid operation

And then update the PR description to reflect the current code state? That's what end up as the body of the commit on main.
Thanks.

Done @gforsyth

@gforsyth gforsyth merged commit 2f85a42 into ibis-project:main Mar 7, 2024
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants