Add match_phrase and slop optional parameter to SQL parser #44
Conversation
Signed-off-by: Yury Fridlyand <[email protected]>
…ents. Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
Signed-off-by: Yury Fridlyand <[email protected]>
|
@Yury-Fridlyand , FYI, SQL Java CI tasks are failing because PrettyFormatResponseIT test uses match_phrase. It should start passing once FilterQueryBuilder is updated. |
| private void generateAndTestQuery(String function, HashMap<String, Object[]> functionArgs) { | ||
| var rand = new Random(); | ||
|
|
||
| for (int i = 0; i < 100; i++) |
There was a problem hiding this comment.
The test should exercise all combinations that are worth testing.
We can end up with a non-deterministic unit test if there are more than 100 test cases and there's a problem with one of them.
| assertNotNull(parser.parse("SELECT * FROM test WHERE match_phrase(column, 100500)")); | ||
| } | ||
|
|
||
| private void generateAndTestQuery(String function, HashMap<String, Object[]> functionArgs) { |
There was a problem hiding this comment.
Nice test generator!
It's pretty complex -- let's make it easier to understand.
In this PR I'll comment on this test specifically.
As a separate task, I'd like to update it to use parametrized tests and to also use it for PPL.
| var args = new ArrayList<String>(); | ||
| for (var pair : functionArgs.entrySet()) | ||
| { | ||
| if (rand.nextBoolean()) |
There was a problem hiding this comment.
What does this if-else control exactly?
| if (rand.nextBoolean()) | ||
| { | ||
| var arg = new StringBuilder(); | ||
| arg.append(rand.nextBoolean() ? "," : ", "); |
There was a problem hiding this comment.
Either "," or ", " are sufficient. Currently, it is exercising the existing lexer as opposed to the parser changes.
| Collections.shuffle(args); | ||
| for (var arg : args) | ||
| query.append(arg); | ||
| query.append(rand.nextBoolean() ? ")" : ");"); |
There was a problem hiding this comment.
I'd skip switching between ")" and ");".
This is testing area of the parser that we did not affect.
| arg.append(rand.nextBoolean() ? pair.getKey().toLowerCase() : pair.getKey().toUpperCase()); | ||
| arg.append(rand.nextBoolean() ? "=" : " = "); | ||
| if (pair.getValue() instanceof String[] || rand.nextBoolean()) { | ||
| var quoteSymbol = rand.nextBoolean() ? '\'' : '"'; |
There was a problem hiding this comment.
Let's pick either single or double quote and stick to that.
|
|
||
| generateAndTestQuery("match", matchArgs); | ||
|
|
||
| var matchPhraseArgs = new HashMap<String, Object[]>(); |
There was a problem hiding this comment.
This would be a good place to use ImmutableMap builder -- https://guava.dev/releases/19.0/api/docs/com/google/common/collect/ImmutableMap.Builder.html
SQL plugin already depends on guava.
match_phrasefunction in SQL parsermatchandmatch_phrasefunctionsSigned-off-by: Yury Fridlyand [email protected]