Repository navigation
[POC] Bound Expressions SerDe - #270
singhpk234 wants to merge 12 commits into
Conversation
singhpk234
changed the title
singhpk234
changed the title
|
cc @rdblue @stevenzwu please have a pass when you get some time |
| BoundPredicate<?> boundPred = (BoundPredicate<?>) boundExpr; | ||
|
|
||
| UnboundTerm<?> unbound = ExpressionUtil.unbind(boundPred.term()); | ||
| assertThat(unbound).isInstanceOf(NamedReference.class); |
There was a problem hiding this comment.
Ideally, unbind can reconstruct the ResolvedReference. not sure it is feasible or not.
There was a problem hiding this comment.
Agree, let me double confirm where the unbind is used, since we have BoundReference and all we care about is an unbound term so both NameReference and ResolvedReference would work
There was a problem hiding this comment.
I would not change the behavior of unbind right now. We don't know the intent of unbinding a reference. It could easily be that you want to go back to binding by name instead of binding by ID.
d3745b6 to
870d601
Compare
|
chaning the NamedReference to UnboundRefernce breaks the rev-api i will adjust the rev-api given we want to move forward with this approach. |
c4a8f4c to
93252f5
Compare
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public static <S, T> UnboundTerm<T> bucket(ResolvedReference<S> resolvedRef, int numBuckets) { |
There was a problem hiding this comment.
There are already transform functions that accept UnboundTerm. Why are these needed?
There was a problem hiding this comment.
public static <T> UnboundTerm<T> bucket(String name, int numBuckets) {
Transform<?, T> transform = (Transform<?, T>) Transforms.bucket(numBuckets);
return new UnboundTransform<>(ref(name), transform);
}
The signatures for those are tied to the String name, i can move this the UnboundTerm as even in this function calls ref(name) which returns NamedReference
this way we have just one api and the caller needs to make sure it passes appropriate unboundTerm
| case REFERENCE: | ||
| return Expressions.ref(JsonUtil.getString(TERM, node)); | ||
| // Reference must have a name (via "name" or "term") | ||
| // fieldId is optional - if present, creates ResolvedReference, otherwise NamedReference |
There was a problem hiding this comment.
Is this correct? Currently, there are three cases for a term:
- A string name:
"term": "some_column", - A reference object:
"term": { "type": "reference", "term": "some_column" }, - A transform object:
"term": { "type": "transform", "term": (a term) }
I think this needs to be updated so that an ID can be optionally added to case 2: "term": { "type": "reference", "field-id": 12, "term": "some_column" }. I can see the appeal of using "name" instead of "term", but reusing term makes this more consistent. We still have 3 cases and are adding optional data, rather than having 4 cases, where two of those cases share a "type".
About the change
The main problem for the bound reference is the accessor, we can't store it in a language agnostic way but we need bound Expressions portability both for ReadRestrictions as well as new adaptive metadata tree.
This pr introduces couple of new APIs :
so a bound expression when serialized is converted to UnboundExpression which contains the ResolvedReference and ResolvedExpression
This is still a work in progress change will keep this update as i discover / break things along the way !