[FLINK-39396][table] Simplify early return condition in SqlLikeChainC…#27894
[FLINK-39396][table] Simplify early return condition in SqlLikeChainC…#27894Au-Miner wants to merge 2 commits intoapache:masterfrom
Conversation
| DataTypes.STRING(), | ||
| DataTypes.STRING()) | ||
| // Empty strings in pattern or escape | ||
| .testSqlResult("f0 LIKE 'test\"end' ESCAPE ''", false, DataTypes.BOOLEAN()) |
There was a problem hiding this comment.
what is the reason of this line removal?
There was a problem hiding this comment.
This line was not deleted, but placed on line 153
This is arranged according to the pattern-ESCAPE-escape format, with three tests being
empty ESCAPE empty
empty ESCAPE non-empty
non-empty ESCAPE empty
| .testSqlResult("f0 LIKE '' ESCAPE '!'", false, DataTypes.BOOLEAN()) | ||
| .testSqlResult("f0 LIKE 'test\"end' ESCAPE ''", false, DataTypes.BOOLEAN()) | ||
| // Escaped _ in quick path: startsWith (BEGIN_PATTERN) | ||
| .testSqlResult("f2 LIKE 'te!_%' ESCAPE '!'", true, DataTypes.BOOLEAN()) | ||
| .testSqlResult("f0 LIKE 'te!_%' ESCAPE '!'", false, DataTypes.BOOLEAN()) | ||
|
|
||
| // Escaped _ in quick path: endsWith (END_PATTERN) | ||
| .testSqlResult("f2 LIKE '%!_st' ESCAPE '!'", true, DataTypes.BOOLEAN()) | ||
|
|
||
| // Escaped _ in quick path: contains (MIDDLE_PATTERN) | ||
| .testSqlResult("f2 LIKE '%!_s%' ESCAPE '!'", true, DataTypes.BOOLEAN()) | ||
|
|
||
| // Escaped _ in quick path: multi-segment (ChainChecker) | ||
| .testSqlResult("f2 LIKE 'te!_%st' ESCAPE '!'", true, DataTypes.BOOLEAN()) | ||
| .testSqlResult("f0 LIKE 'te!_%st' ESCAPE '!'", false, DataTypes.BOOLEAN()) |
There was a problem hiding this comment.
does any of this test fail without a fix?
There was a problem hiding this comment.
At present, this is not a bug fix, but a technical debt
These tests are designed to increase the coverage of the tests
| private final boolean leftAnchor; | ||
| private final boolean rightAnchor; |
There was a problem hiding this comment.
after this change these 2 fields are used only in constructor
do we still need to have them as fields?
There was a problem hiding this comment.
I don't think this is necessary, they have already been moved to SqlLikeChainChecker
| private final int endLen; | ||
| private final boolean leftAnchor; | ||
| private final boolean rightAnchor; | ||
| private final boolean emptyPattern; |
There was a problem hiding this comment.
| private final boolean emptyPattern; | |
| private final boolean isEmptyPattern; |
|
Thanks for the review, let me provide some responses. |
3 participants
What is the purpose of the change
Simplify the early return condition in
SqlLikeChainChecker.check()by replacing a complex 7-line compound expression with a singleemptyPatternboolean flag, improving readability without changing semantics.Brief change log
emptyPatternfield toSqlLikeChainCheckerand simplified the early return condition tomark < minLen || mark > 0 && emptyPattern_going through all quick path branches inLikeFunctionITCaseVerifying this change
This change is already covered by existing tests in
LikeFunctionITCase.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation