Hack to fix some problematic queries #13174

gortiz · 2024-05-17T14:49:26Z

Queries like:

WITH CTE_B AS (
  SELECT 1 AS __ts FROM a GROUP BY __ts
) 
SELECT 1 
FROM CTE_B 
WHERE __ts >= 1 AND __ts < 2

Fail when they are executed. The reason is that very tricky, but mostly is due to:

Our rules are not able to get rid of the tautology (1 >= 1 and 1 < 2 is always true), so it is pushed into the leaf stage
The leaf stage generates an equivalent V1 query, but does not apply CompileTimeFunctionsInvoker, so these tautologies are not removed from the expression tree
V1 row-by-row engine doesn't know how to execute these tautologies because it only expects expressions like col1 > doubleLiteral.

These tautologies are generated in PinotFilterExpandSearchRule. We have two rules that should ideally remove them:

PinotEvaluateLiteralRule (which evaluates the expressions using V1 semantics)
CoreRules.FILTER_REDUCE_EXPRESSIONS (which evalautes the expressions using Calcite semantics)

Alternatively, we should be able to call CompileTimeFunctionsInvoker rewriter in the leaf stage. This invoker is also using V1 semantics.

But any of these changes works.

PinotEvaluateLiteralRule and CompileTimeFunctionsInvoker partially work because V1 can execute = assuming each operand is a number. Things like 1=1 or '123'='123' but cannot execute 'a' = 'a'.
CoreRules.FILTER_REDUCE_EXPRESSIONS can get rid of the tautologies, but in some scenarios it creates just another SEARCH operation. V1 cannot execute them.

I'm not proud of the solution found in this PR. It basically consist of applying the pair PinotFilterExpandSearchRule, CoreRules.FILTER_REDUCE_EXPRESSIONS twice. Although it seems to be working in all cases I tried, I'm sure there will be cases were this solution is not good enough.

IMO it would be better to:

Have a centralized function register, as suggested by @walterddr long ago.
Support search in V1.

Do you have other ideas on how to fix this problem?

gortiz · 2024-05-17T14:50:46Z

...ry-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotFilterExpandSearchRule.java

+/**
+ * Rule to expand search condition in filter.
+ * <p>
+ * For example, the filter condition:
+ * <p>
+ *   <code>SEARCH(col1, Sarg[[1..2)])</code>
+ * </p>
+ * Is expanded to:
+ * <p>
+ *   <code>AND(>=(col1, 1), <(col1, 2))</code>
+ *
+ */


Boy Scout Rule

gortiz · 2024-05-17T14:52:07Z

pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java

 * <p>It provide the higher level entry interface to convert a SQL string into a {@link DispatchableSubPlan}.
 */
 public class QueryEnvironment {
+  private static final Logger LOGGER = LoggerFactory.getLogger(QueryEnvironment.class);


I've added a bunch of logs to better understand whether the tautologies were removed by the validator or the rules. In the cases I mention it is clear it is always the rules, but it was useful to detect other cases where I actually include the tautology in the SQL request

All logs are in debug mode, so they shouldn't be enabled by default.

gortiz · 2024-05-17T14:53:59Z

pinot-query-planner/src/test/resources/log4j2.xml

+<!--      Change FULL_PLAN marker from onMatch="DENY" to onMatch='ACCEPT" to see the full plan before and after each
+          rule that modifies the plan is applied -->
+<!--      <MarkerFilter marker="FULL_PLAN" onMatch="ACCEPT" onMismatch="NEUTRAL"/>-->
+      <MarkerFilter marker="FULL_PLAN" onMatch="DENY" onMismatch="NEUTRAL"/>
+      <AppenderRef ref="console"/>


I found this very useful to actually see what was going on in the rules. ACCEPT is better, but very verbose. I think it could be useful to have in our CI tests, but I will use DENY for a while to be sure we don't spam too much.

BTW, with this log I discovered that PinotRelDistributionTraitRule is being applied multiple times, which may introduce some extra latency in very large plans.

gortiz · 2024-05-17T14:54:37Z

pinot-query-planner/src/test/resources/queries/LiteralEvaluationPlans.json

+        "sql": "EXPLAIN PLAN FOR WITH CTE_B AS (SELECT 1692057600000 AS __ts FROM a GROUP BY __ts) SELECT 1692057600000 AS __ts FROM CTE_B WHERE __ts >= 1692057600000 GROUP BY __ts",
+        "output": [
+          "Execution Plan",
+          "\nLogicalProject(__ts=[1692057600000:BIGINT])",
+          "\n  LogicalAggregate(group=[{0}])",
+          "\n    PinotLogicalExchange(distribution=[hash[0]])",
+          "\n      LogicalAggregate(group=[{0}])",
+          "\n        LogicalProject(__ts=[1692057600000:BIGINT])",
+          "\n          LogicalTableScan(table=[[default, a]])",
+          "\n"


This query works in master. Our PinotEvaluateLiteralRule.Filter.INSTANCE is able to get rid of it.

But for example a query like:

WITH CTE_B AS ( SELECT 'a' AS __ts FROM a GROUP BY __ts ) SELECT 1 FROM CTE_B WHERE __ts >= `b`

Fails because PinotEvaluateLiteralRule.Filter.INSTANCE tries to apply >= in V1, which only works with numbers!

This should no longer be an issue (#13711).

gortiz · 2024-05-17T14:58:11Z

pinot-query-planner/src/test/resources/queries/LiteralEvaluationPlans.json

+        "sql": "EXPLAIN PLAN FOR WITH CTE_B AS (SELECT 1692057600000 AS __ts FROM a GROUP BY __ts) SELECT 1692057600000 AS __ts FROM CTE_B WHERE __ts >= 1692057600000 AND __ts < 1693267200000 GROUP BY __ts",
+        "output": [
+          "Execution Plan",
+          "\nLogicalProject(__ts=[1692057600000:BIGINT])",
+          "\n  LogicalAggregate(group=[{0}])",
+          "\n    PinotLogicalExchange(distribution=[hash[0]])",
+          "\n      LogicalAggregate(group=[{0}])",
+          "\n        LogicalProject(__ts=[1692057600000:BIGINT])",
+          "\n          LogicalTableScan(table=[[default, a]])",
+          "\n"


This is similar to the query in the PR description. It can be fixed by applying PinotEvaluateLiteralRule after PinotFilterExpandSearchRule or adding CompileTimeFunctionsInvoker into ServerPlanRequestUtils.QUERY_REWRITERS.

gortiz · 2024-05-17T14:59:47Z

pinot-query-planner/src/test/resources/queries/LiteralEvaluationPlans.json

+        "description": "Search + OR filter on constants is simplified",
+        "sql": "EXPLAIN PLAN FOR WITH tmp2 AS (SELECT CASE WHEN col2 = 'VAL1' THEN 'A' ELSE col2 END AS cased FROM a) SELECT 1 FROM tmp2 WHERE ((cased = 'B') OR (cased = 'A'))",
+        "output": [
+          "Execution Plan",
+          "\nLogicalProject(EXPR$0=[1])",
+          "\n  LogicalFilter(condition=[SEARCH($1, Sarg[_UTF-8'A':VARCHAR CHARACTER SET \"UTF-8\", _UTF-8'B':VARCHAR CHARACTER SET \"UTF-8\", _UTF-8'VAL1':VARCHAR CHARACTER SET \"UTF-8\"]:VARCHAR CHARACTER SET \"UTF-8\")])",
+          "\n    LogicalTableScan(table=[[default, a]])",
+          "\n"


This query fails in master and cannot be solved by applying PinotEvaluateLiteralRule after PinotFilterExpandSearchRule or adding CompileTimeFunctionsInvoker into ServerPlanRequestUtils.QUERY_REWRITERS.

But it works if we apply CoreRules.FILTER_REDUCE_EXPRESSIONS followed by PinotFilterExpandSearchRule twice in a row.

codecov-commenter · 2024-05-17T15:37:42Z

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 35.09%. Comparing base (59551e4) to head (a163afd).
Report is 458 commits behind head on master.

Files	Patch %	Lines
.../java/org/apache/pinot/query/QueryEnvironment.java	73.33%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #13174       +/-   ##
=============================================
- Coverage     61.75%   35.09%   -26.67%     
+ Complexity      207        6      -201     
=============================================
  Files          2436     2445        +9     
  Lines        133233   134648     +1415     
  Branches      20636    20843      +207     
=============================================
- Hits          82274    47249    -35025     
- Misses        44911    83919    +39008     
+ Partials       6048     3480     -2568

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration	`<0.01% <0.00%> (-0.01%)`	⬇️
integration1	`<0.01% <0.00%> (-0.01%)`	⬇️
integration2	`0.00% <0.00%> (ø)`
java-11	`<0.01% <0.00%> (-61.71%)`	⬇️
java-21	`35.09% <73.33%> (-26.54%)`	⬇️
skip-bytebuffers-false	`35.07% <73.33%> (-26.68%)`	⬇️
skip-bytebuffers-true	`35.07% <73.33%> (+7.34%)`	⬆️
temurin	`35.09% <73.33%> (-26.67%)`	⬇️
unittests	`46.55% <73.33%> (-15.20%)`	⬇️
unittests1	`46.55% <73.33%> (-0.34%)`	⬇️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

It is very verbose

Hack to fix some problematic queries

57986f2

gortiz requested review from Jackie-Jiang and mayankshriv May 17, 2024 14:49

gortiz commented May 17, 2024

View reviewed changes

gortiz added 2 commits May 17, 2024 19:15

Remove RelOptPlanner logs in tests

ef45457

It is very verbose

Fix one of the new queries, which expected the old (and incorrect) plan

a163afd

gortiz marked this pull request as ready for review June 24, 2024 13:41

yashmayya mentioned this pull request Sep 19, 2024

Apply filter reduce expressions Calcite rule at the end to prevent literal-only filter pushdown to leaf stage #14025

Closed

gortiz mentioned this pull request Sep 19, 2024

Query execution error when filtering constant columns from subquery #14010

Closed

yashmayya mentioned this pull request Nov 14, 2024

Apply filter reduce expressions Calcite rule at the end to prevent literal-only filter pushdown to leaf stage #14448

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Hack to fix some problematic queries #13174

Hack to fix some problematic queries #13174

Uh oh!

gortiz commented May 17, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

gortiz May 17, 2024 •

edited

Loading

Uh oh!

yashmayya Oct 8, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

gortiz May 17, 2024

Uh oh!

codecov-commenter commented May 17, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Hack to fix some problematic queries #13174

Are you sure you want to change the base?

Hack to fix some problematic queries #13174

Uh oh!

Conversation

gortiz commented May 17, 2024

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashmayya Oct 8, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

gortiz May 17, 2024

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gortiz May 17, 2024 •

edited

Loading

codecov-commenter commented May 17, 2024 •

edited

Loading