Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Aug 21, 2022

This adds a JSON parser for Expression based on @nastra's work in #5511, but with a few changes:

  1. The format matches what was discussed in Core: add parsers for events and unbounded expressions #4308
  2. Adds a CustomOrderExpressionVisitor to visit expressions when converting to JSON
  3. Transforms are supported if a schema is passed to fromJson
  4. Both bound and unbound expressions can be passed to toJson. Using a bound expression ensures that the JSON representation for values matches expression's type

Closes #4308
Closes #5511

Co-authored-by: Eduard Tudenhoefner [email protected]

BoundReference(Types.NestedField field, Accessor<StructLike> accessor, String name) {
this.field = field;
this.accessor = accessor;
this.name = name;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking the field name was needed to handle bound expressions. This is also a good thing to track for other purposes (e.g., easier to read log messages with bound expressions).

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be good to add a comment to distinguish this name from the field name.

Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

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

+1 LGTM. There might be a couple places where we can remove casts with the addition of name to the reference interface, but I only found one example.

Thanks everyone who contributed!

@rdblue rdblue merged commit 7f2bd24 into apache:master Aug 22, 2022
@rdblue
Copy link
Contributor Author

rdblue commented Aug 22, 2022

Thanks for reviewing, @danielcweeks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants