Skip to content

Match can't be used after STATS .. BY until filters can be pushed down #116642

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.

Sign up for GitHub

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

Merged
carlosdelest merged 1 commit into from
Nov 14, 2024
Merged

Match can't be used after STATS .. BY until filters can be pushed down #116642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,7 +688,7 @@ private static void checkFullTextQueryFunctions(LogicalPlan plan, Set<Failure> f
plan,
condition,
Match.class,
lp -> (lp instanceof Limit == false),
lp -> (lp instanceof Limit == false) && (lp instanceof Aggregate == false),
Copy link
Contributor

@ioanatia ioanatia Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me - but in these types of situations I prefer to have an "allow list" of commands that can be used before match, rather than check for the ones that cannot be used.
This is what we do for qstr and IMO it's a bit safer, because we do not have to worry when we introduce a new command that we need to immediately update this check here. Worst case when we add a new command we have a limitation in place that we can lift any time.
The other way around, where we have a "deny" list like we do now, when we add a new command, the worst case is that the ES|QL query fails in a way we did not predict.

Copy link
Member Author

@carlosdelest carlosdelest Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. As this is temporary until #115311 is fixed, I can revisit the implementation when we change this again.

m -> "[" + m.functionName() + "] " + m.functionType(),
failures
);
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/VerifierTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,6 +1155,10 @@ public void testMatchFunctionNotAllowedAfterCommands() throws Exception {
"1:24: [MATCH] function cannot be used after LIMIT",
error("from test | limit 10 | where match(first_name, \"Anna\")")
);
assertEquals(
"1:47: [MATCH] function cannot be used after STATS",
error("from test | STATS c = AVG(salary) BY gender | where match(gender, \"F\")")
);
}

public void testMatchFunctionAndOperatorHaveCorrectErrorMessages() throws Exception {
Expand Down