Repository navigation
Core: Update Expressions to match Expressions spec - #17343
singhpk234 wants to merge 8 commits into
Conversation
singhpk234
changed the title
e681062 to
134e03b
Compare
There was a problem hiding this comment.
Thanks @singhpk234, I'm still going through the change in more depth but I think the main question I have is around compatibility in terms of older readers. Currently, the change will serialize in the new spec compliant representation but older readers wouldn't be able to understand it. Do we need to have a separate serialization mode that produces older compatible expressions in the APIs? cc @rdblue @danielcweeks
I know there's also been discussion around loadTableV2 which we'd need for other use cases like V4 etc, so one path is that those APIs are bound to using the newer expression format, and clients would anyways need to handle the new format for those newer APIs etc. But from a core expression library perspective we may need to expose some way to not just always produce the newer expression representation. Use cases that need to produce older-compatible expressions, can use that API, use cases that can produce newer metadata can just do that.
| this.gen = gen; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Not sure this needed to be removed?
6f42396 to
6037638
Compare
| * @param <T> the Java type of this term | ||
| * @return an unbound apply expression | ||
| */ | ||
| public static <T> UnboundApply<T> apply(FunctionReference function, List<Object> arguments) { |
There was a problem hiding this comment.
The other methods look good, but I think List<Object> is too broad here. This helper may need to be omitted until we have the type hierarchy built out with a type for ValueExpression + Predicate, which I think is called FUNC_ARG in the spec (maybe FunctionArg here?).
I think @danielcweeks has been working on the hierarchy changes.
There was a problem hiding this comment.
This helper may need to be omitted until we have the type hierarchy built out with a type for ValueExpression + Predicate, which I think is called FUNC_ARG in the spec (maybe FunctionArg here?).
precisely, hence i couldn't find some thing better than Object, should i make this package private (so its not a public api ? ) and meanwhile when we have the hierarchy changes we could remove this ?
There was a problem hiding this comment.
maybe FunctionArg here
Function Arg would be nice abstraction !
will let Dan chime in too, for now for the completeness of impl i just marked them as private, this can come as follow up as well and i am fine to remove this completely as well
| * | ||
| * @param <T> the Java type of values produced by this expression | ||
| */ | ||
| public class UnboundApply<T> implements UnboundTerm<T> { |
There was a problem hiding this comment.
I think that this should be a ValueExpression type that it not a Term. Term is used for expressions that return a value based on a reference, but ValueExpression will be more broad. It will include literals (not Term) and these Apply expressions.
singhpk234
changed the title
1da0c08 to
ef35503
Compare
Adds FunctionReference and UnboundApply, and Expressions factory methods to build function application terms from the expressions spec.
Serializes predicates in the form defined in format/expressions-spec.md, including apply terms and field-id references, while continuing to accept the deprecated term-based form.
Keeps the original structure of the unbound predicate writer, stores apply arguments in an array so the class is Kryo serializable, and splits the null-catalog function reference check into its own test. Generated-by: Claude Code
Adds BoundApply so that an UnboundApply with a known result type can be bound by binding each of its arguments. Bound applies can be written to JSON and unbound. Applies without a result type, such as those read from JSON, still cannot be bound. Generated-by: Claude Code
The expressions spec defines partition transforms as functions in the iceberg_functions catalog, and a function reference without a catalog refers to the catalog of the referencing object. Only read applies as transforms when they use the iceberg_functions catalog, and write transforms with it. Generated-by: Claude Code
The expressions spec defines partition transforms as functions in the iceberg_functions catalog. UnboundTransform and BoundTransform now extend UnboundApply and BoundApply, so the parser writes them through the same path as other function applications. Generated-by: Claude Code
Removes Expressions.apply and makes UnboundApply.arguments package-private so function arguments typed as Object are not public API until a function argument type exists. Stores function reference identifiers in an array for Kryo, and trims redundant comments and code. Generated-by: Claude Code
…on format Generated-by: Claude Code
ef35503 to
96820a1
Compare
Summary
Updates
ExpressionParserto the JSON serialization in the expressions spec (#16652). This is the Java side of #17138.Changes
API
FunctionReferenceandUnboundApplyfor function calls in expressions.Expressions.function(...)andExpressions.apply(...).Core
left/right/childoperands,{"type":"reference","name"|"id"}, and transforms asapply, e.g.bucket(16, id).identity,year,month,day,hour,bucket,truncate) are read back asUnboundTransform. Other functions stayUnboundApplyand cannot be bound yet.fromJson.data-type) and theliteralsset form.Compatibility
The writer always emits the new form. Readers from before this change cannot parse it.