Skip to content

Conversation

@rdblue
Copy link

@rdblue rdblue commented Aug 14, 2022

This is a slightly different approach to ExpressionParser than in apache#5511.

First, this adds CustomOrderExpressionVisitor to separate the logic to traverse an expression from the logic to produce JSON from an expression node.

Second, this updates the parser to use the serialization that was proposed in apache#4308 because there were some slight differences, like using operand instead of child in Not.

Updating to the proposed JSON format introduced new challenges. The proposed format requires serializing expression values using single-value JSON serialization, but the existing parser for that serialization format requires a Type to decide how to serialize. To get a type, expressions must be bound before serializing.

Binding an expression before serialization is probably a good idea because that will catch any issues with an unbound expression before sending it. If the expression can't be bound, it usually doesn't make sense to allow serializing it. However, it does make sense to serialize expressions that can't be bound on the remote side in some cases, like after an expression is sanitized and all the values are replaced with string descriptions (like "(16-digit-int)").

This implements bound expression serialization and deserialization, which requires passing a schema. Passing a schema allows using the DefaultValueParser for values and also works around the problem of deserializing transforms because the type can be known when deserializing.

@rdblue rdblue force-pushed the expression-parser branch from d02d211 to e420439 Compare August 14, 2022 20:49
@nastra nastra closed this Sep 28, 2022
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.

2 participants