multi-value string expression transformation fix#8019
Merged
fjy merged 8 commits intoapache:masterfrom Jul 4, 2019
Merged
Conversation
jon-wei
reviewed
Jul 4, 2019
| this.rootNodeKey = rootNodeKey; | ||
| this.macroTable = macroTable; | ||
| this.nodes = new HashMap<>(); | ||
| this.lambadIdentifiers = new HashSet<>(); |
| } | ||
| List<String> unapplied = toApply.stream() | ||
| .filter(x -> bindingDetails.getFreeVariables().contains(x)) | ||
| .filter(x -> bindingDetails.getRequiredColumnsList().contains(x)) |
Contributor
There was a problem hiding this comment.
Suggest using getRequiredColumns here, since contains should be cheaper on the Set vs ArrayList if there are lots of columns (and skip the ArrayList creation too)
Member
Author
There was a problem hiding this comment.
Oops, getRequiredColumns originally returned a list, but I changed it to getRequiredColumnsList and added getRequiredColumns to return a set for this reason but forgot to make sure I updated all the places.
| } | ||
|
|
||
| /** | ||
| * Set of identifiers which are used with scalar operators and functions |
Contributor
There was a problem hiding this comment.
Suggest adding more docs here about how these differ from the get*Variables() methods
Member
Author
There was a problem hiding this comment.
Updated javadoc to specify if returning IdentifierExpr.identifier or IdentifierExpr.bindingIdentifier
gianm
pushed a commit
to implydata/druid-public
that referenced
this pull request
Jul 4, 2019
* multi-value string expression transformation fix * fixes * more docs and test * revert unintended doc change * formatting * change tostring to print binding identifier * review fixup * oops
This was referenced Nov 27, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue with the automatic translation of expressions for multi-value string column inputs, where if an identifier was duplicated, the expression would not transform correctly.
This has been resolved by modifying expression parsing to uniquely identify all non lambda bound
IdentifierExpr, at parsing transformation time inExprListenerImpl.LambdaExpridentifiers are marked on entering a lambda expression, and all non-lambda identifiers are given a unique identifier to accompany the identifier to use to get the value from theExpr.ObjectBinding.Expr.BindingDetailshas been reworked to accommodate this change, as well as simplified. Further refactoring took place inParserto handle the transformation required to createLambdaExprbindingIdentifierExprto ensure the correct transformation.Example:
before patch:
where
xis a multi-value string dimension would result in a transformation to something likeafter patch:
this will correctly map to something like
This PR has: