-
Notifications
You must be signed in to change notification settings - Fork 5
Variant support V1 #706
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
Variant support V1 #706
Conversation
rampage644
left a comment
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.
Didn't dive deep into actual udf implemenetation, overall structure overall lgtm
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.
Pull Request Overview
This PR introduces variant object manipulation functions and refactors existing variant functions and module structures for improved organization and type safety. Key changes include:
- Updates to the module paths for UDFs and UDAFs, e.g. moving functions into dedicated udafs and udfs folders.
- The addition of new dependency jsonpath_lib and a new lint rule in Cargo.toml.
- Removal of the aggregate/mod.rs file and adjustments in snapshot metadata.
Reviewed Changes
Copilot reviewed 73 out of 73 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/runtime/src/execution/datafusion/functions/udfs/date_from_parts.rs | Updated import paths to reflect new module structure for UDFs. |
| crates/runtime/src/execution/datafusion/functions/udafs/mod.rs | Introduced module to register UDAFs. |
| crates/runtime/src/execution/datafusion/functions/udafs/any_value.rs | Removed the Apache license header and added an attribute to the new constructor. |
| crates/runtime/src/execution/datafusion/functions/table/flatten.rs | Adjusted test import paths to match the new UDF module structure. |
| crates/runtime/src/execution/datafusion/functions/mod.rs | Restructured module exports to separate udafs and udfs. |
| crates/runtime/src/execution/datafusion/functions/aggregate/mod.rs | Removed the aggregate module likely due to refactoring consolidations. |
| crates/runtime/Cargo.toml | Added new dependency jsonpath_lib. |
| crates/metastore/src/snapshots/embucket_metastore__metastore__tests__create_volumes.snap | Updated snapshot metadata by adding snapshot_kind. |
| Cargo.toml | Added a new lint rule to allow redundant_closure_for_method_calls. |
Comments suppressed due to low confidence (1)
crates/runtime/src/execution/datafusion/functions/udafs/any_value.rs:1
- The Apache license header has been removed in this file. Please ensure that the appropriate licensing text is maintained according to project licensing policies.
// Licensed to the Apache Software Foundation (ASF) under one
|
@eadgbear do you plan to resolve conflicts? |
|
@ravlio What was the reason for moving all of the functions to a new crate? It's now made this PR even bigger with the merge |
|
The decision to move functions into a separate crate followed as a natural consequence of the #744. |
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.
To make sure this won't accidentally get merged without a rebase
|
@eadgbear I found such missing functions: array_construct_compact If you don't mind, I'll take them to work |
That one I think is achieved via AST rewrite rule btw |
|
Array to string is redundant as it's already a string but can be rewritten
as a NO-OP
…On Wed, May 14, 2025, 09:30 Sergei Turukin ***@***.***> wrote:
*rampage644* left a comment (Embucket/embucket#706)
<https://github.com/Embucket/embucket/pull/706#issuecomment-2880856279>
array_construct_compact
That one I think is achieved via AST rewrite rule btw
—
Reply to this email directly, view it on GitHub
<https://github.com/Embucket/embucket/pull/706#issuecomment-2880856279>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKDOYVIF2VTJBSQMXI4TD26NVRFAVCNFSM6AAAAAB4QJDOFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQOBQHA2TMMRXHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@eadgbear the thing is that not a big deal to implement though, just replace |
|
Ah got it. Missed that
…On Wed, May 14, 2025, 09:44 Maxim Bogdanov ***@***.***> wrote:
*ravlio* left a comment (Embucket/embucket#706)
<https://github.com/Embucket/embucket/pull/706#issuecomment-2880892570>
@eadgbear <https://github.com/eadgbear> the thing is that array_string
has separator argument:
ARRAY_TO_STRING( <array> , <separator_string> )
not a big deal to implement though, just replace , to <separator_string>,
but anyway let me implement this
—
Reply to this email directly, view it on GitHub
<https://github.com/Embucket/embucket/pull/706#issuecomment-2880892570>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKDO2UAFEMJRYFP77ODID26NXG7AVCNFSM6AAAAAB4QJDOFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQOBQHA4TENJXGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Latest commit: Adding object functions v1
object_delete: Remove elements from variant objectsobject_insert: Add new elements to variant objectsobject_pick: Select specific elements from variant objectsCode standards alignment
Initial variant function implementation
TODO
object_constructneeds more work and will arrive in a subsequent PR