-
Couldn't load subscription status.
- Fork 176
Add replace command with Calcite #4451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3621bc9 to
b4fe742
Compare
| @ToString | ||
| @EqualsAndHashCode(callSuper = false) | ||
| public class Replace extends UnresolvedPlan { | ||
| private final UnresolvedExpression pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We should annotate these with @NotNull.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to @Nullable, received similar comment before: #3878 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I also prefer using optional in fields, despite the warning. I'm not sure why that warning exists. 🤷
But why mark it nullable if we validate that they can't be null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We marked child as @nullable, which we didn't include in the validate logic
| UnresolvedExpression pattern = internalVisitExpression(ctx.pattern); | ||
| UnresolvedExpression replacement = internalVisitExpression(ctx.replacement); | ||
|
|
||
| List<Field> fieldList = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Should we have a global method for turning this into a LinkedHashSet?
This is currently checked in validate(), but I don't think this is the only command to rely on a fieldList. If we make Replace take a Set instead of a List, we simplify the validation logic there, and we can push the responsibility of deduplicating fieldLists to shared parsing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to accept Set instead of List, removing duplicate validation logic
| verifyPPLToSparkSQL(root, expectedSparkSql); | ||
| } | ||
|
|
||
| @Test(expected = Exception.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Catch a more specific Exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to be more specific
13fa309 to
414a9f8
Compare
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
Signed-off-by: Kai Huang <[email protected]>
8d93683 to
0ff7d97
Compare
| @Deprecated | ||
| public UnresolvedExpression getReplacement() { | ||
| if (replacePairs.isEmpty()) { | ||
| throw new IllegalStateException("No replacement pairs available"); | ||
| } | ||
| return replacePairs.get(0).getReplacement(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete instead of deprecate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| @Deprecated | ||
| public UnresolvedExpression getPattern() { | ||
| if (replacePairs.isEmpty()) { | ||
| throw new IllegalStateException("No replacement pairs available"); | ||
| } | ||
| return replacePairs.get(0).getPattern(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete instead of deprecate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| public Replace( | ||
| UnresolvedExpression pattern, UnresolvedExpression replacement, Set<Field> fieldList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to remove unused parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes!
Signed-off-by: Kai Huang <[email protected]>
| @Getter | ||
| @AllArgsConstructor | ||
| @EqualsAndHashCode | ||
| @ToString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: consider using @Value
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-4451-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5677765e6d2f0203fc99014e5ebc4aa27424b57d
# Push it to GitHub
git push --set-upstream origin backport/backport-4451-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
* Add replace command with Calcite Signed-off-by: Kai Huang <[email protected]> --------- Signed-off-by: Kai Huang <[email protected]> Co-authored-by: Manasvini B S <[email protected]> (cherry picked from commit 5677765) Signed-off-by: Kai Huang <[email protected]>
* Add replace command with Calcite (#4451) * Add replace command with Calcite Signed-off-by: Kai Huang <[email protected]> --------- Signed-off-by: Kai Huang <[email protected]> Co-authored-by: Manasvini B S <[email protected]> (cherry picked from commit 5677765) Signed-off-by: Kai Huang <[email protected]> * fix compile Signed-off-by: Kai Huang <[email protected]> * backporting backslash handling from main and fix tests Signed-off-by: Kai Huang <[email protected]> * Fix tests Signed-off-by: Kai Huang <[email protected]> * fix tests Signed-off-by: Kai Huang <[email protected]> * compatability accross java versions Signed-off-by: Kai Huang <[email protected]> --------- Signed-off-by: Kai Huang <[email protected]> Co-authored-by: Manasvini B S <[email protected]>
Description
Implement the
replacecommand in PPL to replace text patterns in specified fields. This PR includes the grammar implementation and basic replacement functionality. This implementation reuses the existing replace core logic. Missing / new features for regex support will be added in a separate PR.Original PR: #4248
Syntax
The
replacecommand modifies the specified fields in-place with the replaced text.Semantics
Expected Behavior
Implementation Approach
Example Queries
Output Schema
For each field specified in the
INclause, the field is modified in-place:Example
Input:
After
replace 'error' WITH 'ERROR' IN message, level→Resolves
#3975
Check List
--signoffor-s)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.