From 0033c693b3f17e2ca9d178605b9501486181ac28 Mon Sep 17 00:00:00 2001 From: anlowee Date: Mon, 6 Oct 2025 19:43:50 +0000 Subject: [PATCH 1/9] Add unit tests --- .../optimization/ClpFilterToKqlConverter.java | 33 +++- .../presto/plugin/clp/TestClpPushDown.java | 166 +++++++++++++++++ .../plugin/clp/TestingClpSessionContext.java | 168 ++++++++++++++++++ 3 files changed, 365 insertions(+), 2 deletions(-) create mode 100644 presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java create mode 100644 presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java index f99369b4a48b..f6247e8e4672 100644 --- a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java +++ b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java @@ -16,6 +16,7 @@ import com.facebook.presto.common.function.OperatorType; import com.facebook.presto.common.type.DecimalType; import com.facebook.presto.common.type.RowType; +import com.facebook.presto.common.type.TimestampType; import com.facebook.presto.common.type.Type; import com.facebook.presto.common.type.VarcharType; import com.facebook.presto.plugin.clp.ClpColumnHandle; @@ -57,12 +58,15 @@ import static com.facebook.presto.common.type.IntegerType.INTEGER; import static com.facebook.presto.common.type.RealType.REAL; import static com.facebook.presto.common.type.SmallintType.SMALLINT; +import static com.facebook.presto.common.type.TimestampType.TIMESTAMP; +import static com.facebook.presto.common.type.TimestampType.TIMESTAMP_MICROSECONDS; import static com.facebook.presto.common.type.TinyintType.TINYINT; import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION; import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.AND; import static java.lang.Integer.parseInt; import static java.lang.String.format; import static java.util.Objects.requireNonNull; +import static java.util.concurrent.TimeUnit.SECONDS; /** * A translator to translate Presto {@link RowExpression}s into: @@ -256,8 +260,10 @@ private ClpExpression handleBetween(CallExpression node) } String variable = variableOpt.get(); - String lowerBound = getLiteralString((ConstantExpression) second); - String upperBound = getLiteralString((ConstantExpression) third); + Type lowerBoundType = second.getType(); + String lowerBound = tryEnsureNanosecondTimestamp(lowerBoundType, getLiteralString((ConstantExpression) second)); + Type upperBoundType = third.getType(); + String upperBound = tryEnsureNanosecondTimestamp(upperBoundType, getLiteralString((ConstantExpression) third)); String kql = String.format("%s >= %s AND %s <= %s", variable, lowerBound, variable, upperBound); String metadataSqlQuery = metadataFilterColumns.contains(variable) ? String.format("\"%s\" >= %s AND \"%s\" <= %s", variable, lowerBound, variable, upperBound) @@ -440,6 +446,7 @@ private ClpExpression buildClpExpression( RowExpression originalNode) { String metadataSqlQuery = null; + literalString = tryEnsureNanosecondTimestamp(literalType, literalString); if (operator.equals(EQUAL)) { if (literalType instanceof VarcharType) { return new ClpExpression(format("%s: \"%s\"", variableName, escapeKqlSpecialCharsForStringValue(literalString))); @@ -914,9 +921,31 @@ public static boolean isClpCompatibleNumericType(Type type) || type.equals(TINYINT) || type.equals(DOUBLE) || type.equals(REAL) + || type.equals(TIMESTAMP) + || type.equals(TIMESTAMP_MICROSECONDS) || type instanceof DecimalType; } + private static String tryEnsureNanosecondTimestamp(Type type, String literalString) + { + if (type == TIMESTAMP) { + return ensureNanosecondTimestamp(TIMESTAMP, literalString); + } + else if (type == TIMESTAMP_MICROSECONDS) { + return ensureNanosecondTimestamp(TIMESTAMP_MICROSECONDS, literalString); + } + return literalString; + } + + private static String ensureNanosecondTimestamp(TimestampType type, String literalString) + { + long literalNumber = Long.parseLong(literalString); + long seconds = type.getEpochSecond(literalNumber); + long nanosecondFraction = type.getNanos(literalNumber); + long nanoseconds = SECONDS.toNanos(seconds) + nanosecondFraction; + return Long.toString(nanoseconds); + } + private static class SubstrInfo { String variableName; diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java new file mode 100644 index 000000000000..ab311e818f16 --- /dev/null +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java @@ -0,0 +1,166 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.plugin.clp; + +import com.facebook.presto.dispatcher.DispatchManager; +import com.facebook.presto.execution.QueryManager; +import com.facebook.presto.plugin.clp.metadata.ClpMetadataProvider; +import com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider; +import com.facebook.presto.plugin.clp.mockdb.ClpMockMetadataDatabase; +import com.facebook.presto.plugin.clp.mockdb.table.ColumnMetadataTableRows; +import com.facebook.presto.spi.QueryId; +import com.facebook.presto.spi.plan.PlanNode; +import com.facebook.presto.spi.plan.PlanNodeId; +import com.facebook.presto.spi.plan.TableScanNode; +import com.facebook.presto.tests.DistributedQueryRunner; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import org.intellij.lang.annotations.Language; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.util.Map; +import java.util.Optional; + +import static com.facebook.presto.execution.QueryState.RUNNING; +import static com.facebook.presto.plugin.clp.ClpQueryRunner.createQueryRunner; +import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Boolean; +import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.DateString; +import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Float; +import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Integer; +import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.VarString; +import static java.lang.String.format; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; + +@Test(singleThreaded = true) +public class TestClpPushDown +{ + private static final String TABLE_NAME = "test_pushdown"; + private static final Long TEST_TS_SECONDS = 1746003005L; + private static final Long TEST_TS_NANOSECONDS = 1746003005000000000L; + + private ClpMockMetadataDatabase mockMetadataDatabase; + private DistributedQueryRunner queryRunner; + private QueryManager queryManager; + private DispatchManager dispatchManager; + + @BeforeMethod + public void setUp() throws Exception { + mockMetadataDatabase = ClpMockMetadataDatabase + .builder() + .build(); + mockMetadataDatabase.addTableToDatasetsTableIfNotExist(ImmutableList.of(TABLE_NAME)); + mockMetadataDatabase.addColumnMetadata(ImmutableMap.of(TABLE_NAME, new ColumnMetadataTableRows( + ImmutableList.of( + "city.Name", + "city.Region.id", + "city.Region.Name", + "fare", + "isHoliday", + "ts"), + ImmutableList.of( + VarString, + Integer, + VarString, + Float, + Boolean, + DateString)))); + ClpConfig config = new ClpConfig() + .setPolymorphicTypeEnabled(true) + .setMetadataDbUrl(mockMetadataDatabase.getUrl()) + .setMetadataDbUser(mockMetadataDatabase.getUsername()) + .setMetadataDbPassword(mockMetadataDatabase.getPassword()) + .setMetadataTablePrefix(mockMetadataDatabase.getTablePrefix()); + ClpMetadataProvider metadataProvider = new ClpMySqlMetadataProvider(config); + queryRunner = createQueryRunner( + mockMetadataDatabase.getUrl(), + mockMetadataDatabase.getUsername(), + mockMetadataDatabase.getPassword(), + mockMetadataDatabase.getTablePrefix(), + Optional.of(0), + Optional.empty()); + queryManager = queryRunner.getCoordinator().getQueryManager(); + dispatchManager = queryRunner.getCoordinator().getDispatchManager(); + } + + @AfterMethod + public void tearDown() throws InterruptedException { + long maxCleanUpTime = 5 * 1000L; // 5 seconds + long currentCleanUpTime = 0L; + while (!queryManager.getQueries().isEmpty() && currentCleanUpTime < maxCleanUpTime) { + Thread.sleep(1000L); + currentCleanUpTime += 1000L; + } + if (null != mockMetadataDatabase) { + mockMetadataDatabase.teardown(); + } + } + + public void testTimestampComparisons() + { + testPushDown(format("ts > from_unixtime(%s)", TEST_TS_SECONDS), format("ts > %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts >= from_unixtime(%s)", TEST_TS_SECONDS), format("ts >= %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts < from_unixtime(%s)", TEST_TS_SECONDS), format("ts < %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts <= from_unixtime(%s)", TEST_TS_SECONDS), format("ts <= %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts = from_unixtime(%s)", TEST_TS_SECONDS), format("ts: %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts != from_unixtime(%s)", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); + testPushDown(format("ts <> from_unixtime(%s)", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) < ts", TEST_TS_SECONDS), format("ts > %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) <= ts", TEST_TS_SECONDS), format("ts >= %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) > ts", TEST_TS_SECONDS), format("ts < %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) >= ts", TEST_TS_SECONDS), format("ts <= %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) = ts", TEST_TS_SECONDS), format("ts: %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) != ts", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); + testPushDown(format("from_unixtime(%s) <> ts", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); + } + + private void testPushDown(String filter, String expectedPushDown) { + try { + QueryId id = queryRunner.getCoordinator().getDispatchManager().createQueryId(); + @Language("SQL") String sql = format("SELECT * FROM clp.default.test_pushdown WHERE %s LIMIT 1", filter); + dispatchManager.createQuery( + id, + "slug", + 0, + new TestingClpSessionContext(queryRunner.getDefaultSession()), + sql).get(); + long maxDispatchingAndPlanningTime = 60 * 1000L; // 1 minute + long currentWaitingTime = 0L; + while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() && currentWaitingTime < maxDispatchingAndPlanningTime) { + Thread.sleep(1000L); + currentWaitingTime += 1000L; + } + assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); + boolean isPushDownGenerated = false; + for (Map.Entry entry : queryManager.getFullQueryInfo(id).getPlanIdNodeMap().entrySet()) { + if (!(entry.getValue() instanceof TableScanNode) + || (!(((TableScanNode) entry.getValue()).getTable().getLayout().orElse(null) instanceof ClpTableLayoutHandle))) { + continue; + } + ClpTableLayoutHandle clpTableLayoutHandle = (ClpTableLayoutHandle) ((TableScanNode) entry.getValue()).getTable().getLayout().orElseThrow(AssertionError::new); + String actualPushDown = clpTableLayoutHandle.getKqlQuery().orElse(null); + assertEquals(actualPushDown, expectedPushDown); + isPushDownGenerated = true; + break; + } + assertTrue(isPushDownGenerated); + queryManager.cancelQuery(id); + } catch (Exception e) { + fail(e.getMessage()); + } + } +} diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java new file mode 100644 index 000000000000..883cc8475c6c --- /dev/null +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java @@ -0,0 +1,168 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.facebook.presto.plugin.clp; + +import com.facebook.presto.Session; +import com.facebook.presto.common.RuntimeStats; +import com.facebook.presto.common.transaction.TransactionId; +import com.facebook.presto.server.SessionContext; +import com.facebook.presto.spi.ConnectorId; +import com.facebook.presto.spi.function.SqlFunctionId; +import com.facebook.presto.spi.function.SqlInvokedFunction; +import com.facebook.presto.spi.security.Identity; +import com.facebook.presto.spi.session.ResourceEstimates; +import com.facebook.presto.spi.tracing.Tracer; +import com.google.common.collect.ImmutableMap; + +import java.util.Map; +import java.util.Optional; +import java.util.Set; + +import static java.util.Map.Entry; +import static java.util.Objects.requireNonNull; + +public class TestingClpSessionContext + implements SessionContext +{ + private final Session session; + + public TestingClpSessionContext(Session session) + { + this.session = requireNonNull(session, "session is null"); + } + + @Override + public Identity getIdentity() + { + return session.getIdentity(); + } + + @Override + public String getCatalog() + { + return session.getCatalog().orElse("clp"); + } + + @Override + public String getSchema() + { + return session.getSchema().orElse("default"); + } + + @Override + public String getSource() + { + return session.getSource().orElse(null); + } + + @Override + public Optional getTraceToken() + { + return session.getTraceToken(); + } + + @Override + public String getRemoteUserAddress() + { + return session.getRemoteUserAddress().orElse(null); + } + + @Override + public String getUserAgent() + { + return session.getUserAgent().orElse(null); + } + + @Override + public String getClientInfo() + { + return session.getClientInfo().orElse(null); + } + + @Override + public Set getClientTags() + { + return session.getClientTags(); + } + + @Override + public ResourceEstimates getResourceEstimates() + { + return session.getResourceEstimates(); + } + + @Override + public String getTimeZoneId() + { + return session.getTimeZoneKey().getId(); + } + + @Override + public String getLanguage() + { + return session.getLocale().getLanguage(); + } + + @Override + public Optional getTracer() + { + return session.getTracer(); + } + + @Override + public Map getSystemProperties() + { + return session.getSystemProperties(); + } + + @Override + public Map> getCatalogSessionProperties() + { + ImmutableMap.Builder> catalogSessionProperties = ImmutableMap.builder(); + for (Entry> entry : session.getConnectorProperties().entrySet()) { + catalogSessionProperties.put(entry.getKey().getCatalogName(), entry.getValue()); + } + return catalogSessionProperties.build(); + } + + @Override + public Map getPreparedStatements() + { + return session.getPreparedStatements(); + } + + @Override + public Optional getTransactionId() + { + return session.getTransactionId(); + } + + @Override + public boolean supportClientTransaction() + { + return session.isClientTransactionSupport(); + } + + @Override + public Map getSessionFunctions() + { + return session.getSessionFunctions(); + } + + @Override + public RuntimeStats getRuntimeStats() + { + return session.getRuntimeStats(); + } +} From d2bec6fa041761b0237b2c9cc3a9ea91f6b5f109 Mon Sep 17 00:00:00 2001 From: anlowee Date: Mon, 6 Oct 2025 20:00:03 +0000 Subject: [PATCH 2/9] Add unit tests for BETWEEN and fix some format issues --- presto-clp/pom.xml | 6 ++ .../presto/plugin/clp/TestClpPushDown.java | 55 ++++++++++++------- 2 files changed, 41 insertions(+), 20 deletions(-) diff --git a/presto-clp/pom.xml b/presto-clp/pom.xml index d13b59968aff..7a654371416e 100644 --- a/presto-clp/pom.xml +++ b/presto-clp/pom.xml @@ -105,6 +105,12 @@ test + + com.facebook.presto + presto-main + test + + com.facebook.presto presto-main-base diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java index ab311e818f16..bfc529cd6275 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java @@ -50,8 +50,10 @@ public class TestClpPushDown { private static final String TABLE_NAME = "test_pushdown"; - private static final Long TEST_TS_SECONDS = 1746003005L; - private static final Long TEST_TS_NANOSECONDS = 1746003005000000000L; + private static final Long TEST_TS_SECONDS_LOWER_BOUND = 1746003005L; + private static final Long TEST_TS_NANOSECONDS_LOWER_BOUND = 1746003005000000000L; + private static final Long TEST_TS_SECONDS_UPPER_BOUND = 1746003015L; + private static final Long TEST_TS_NANOSECONDS_UPPER_BOUND = 1746003015000000000L; private ClpMockMetadataDatabase mockMetadataDatabase; private DistributedQueryRunner queryRunner; @@ -59,7 +61,9 @@ public class TestClpPushDown private DispatchManager dispatchManager; @BeforeMethod - public void setUp() throws Exception { + public void setUp() + throws Exception + { mockMetadataDatabase = ClpMockMetadataDatabase .builder() .build(); @@ -98,7 +102,9 @@ public void setUp() throws Exception { } @AfterMethod - public void tearDown() throws InterruptedException { + public void tearDown() + throws InterruptedException + { long maxCleanUpTime = 5 * 1000L; // 5 seconds long currentCleanUpTime = 0L; while (!queryManager.getQueries().isEmpty() && currentCleanUpTime < maxCleanUpTime) { @@ -112,23 +118,31 @@ public void tearDown() throws InterruptedException { public void testTimestampComparisons() { - testPushDown(format("ts > from_unixtime(%s)", TEST_TS_SECONDS), format("ts > %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts >= from_unixtime(%s)", TEST_TS_SECONDS), format("ts >= %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts < from_unixtime(%s)", TEST_TS_SECONDS), format("ts < %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts <= from_unixtime(%s)", TEST_TS_SECONDS), format("ts <= %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts = from_unixtime(%s)", TEST_TS_SECONDS), format("ts: %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts != from_unixtime(%s)", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); - testPushDown(format("ts <> from_unixtime(%s)", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) < ts", TEST_TS_SECONDS), format("ts > %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) <= ts", TEST_TS_SECONDS), format("ts >= %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) > ts", TEST_TS_SECONDS), format("ts < %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) >= ts", TEST_TS_SECONDS), format("ts <= %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) = ts", TEST_TS_SECONDS), format("ts: %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) != ts", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); - testPushDown(format("from_unixtime(%s) <> ts", TEST_TS_SECONDS), format("NOT ts: %s", TEST_TS_NANOSECONDS)); + // Test logical binary + testPushDown(format("ts > from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts > %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts >= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts >= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts < from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts < %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts <= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts = from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts != from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("ts <> from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) < ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts > %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) <= ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts >= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) > ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts < %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) >= ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) = ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) != ts", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + testPushDown(format("from_unixtime(%s) <> ts", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + + // Test BETWEEN + testPushDown(format("ts >= from_unixtime(%s) AND ts <= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND, TEST_TS_SECONDS_UPPER_BOUND), + format("ts >= %s AND ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND, TEST_TS_NANOSECONDS_UPPER_BOUND)); + testPushDown(format("ts BETWEEN from_unixtime(%s) AND from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND, TEST_TS_SECONDS_UPPER_BOUND), + format("ts >= %s AND ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND, TEST_TS_NANOSECONDS_UPPER_BOUND)); } - private void testPushDown(String filter, String expectedPushDown) { + private void testPushDown(String filter, String expectedPushDown) + { try { QueryId id = queryRunner.getCoordinator().getDispatchManager().createQueryId(); @Language("SQL") String sql = format("SELECT * FROM clp.default.test_pushdown WHERE %s LIMIT 1", filter); @@ -159,7 +173,8 @@ private void testPushDown(String filter, String expectedPushDown) { } assertTrue(isPushDownGenerated); queryManager.cancelQuery(id); - } catch (Exception e) { + } + catch (Exception e) { fail(e.getMessage()); } } From a929cd6b0114ab31a99bbf402a8e87657d4589fb Mon Sep 17 00:00:00 2001 From: anlowee Date: Tue, 7 Oct 2025 19:49:39 +0000 Subject: [PATCH 3/9] Move all unit test cases except metadata (split filter) pushdown from TestClpFilterToKql to TestClpPushDown --- .../optimization/ClpFilterToKqlConverter.java | 15 +- .../presto/plugin/clp/TestClpPushDown.java | 368 ++++++++++++++---- 2 files changed, 309 insertions(+), 74 deletions(-) diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java index f6247e8e4672..442ea82a6acd 100644 --- a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java +++ b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java @@ -493,7 +493,7 @@ private Optional tryInterpretSubstringEquality( RowExpression possibleSubstring, RowExpression possibleLiteral) { - if (!operator.equals(EQUAL)) { + if (!operator.equals(EQUAL) && !(operator.equals(NOT_EQUAL))) { return Optional.empty(); } @@ -508,7 +508,7 @@ private Optional tryInterpretSubstringEquality( } String targetString = getLiteralString((ConstantExpression) possibleLiteral); - return interpretSubstringEquality(maybeSubstringCall.get(), targetString); + return interpretSubstringEquality(maybeSubstringCall.get(), targetString, operator.equals(EQUAL)); } /** @@ -561,8 +561,12 @@ private Optional parseSubstringCall(CallExpression callExpression) * @param targetString the literal string being compared to * @return an Optional containing either a ClpExpression with the equivalent KQL query */ - private Optional interpretSubstringEquality(SubstrInfo info, String targetString) + private Optional interpretSubstringEquality(SubstrInfo info, String targetString, boolean isEqual) { + StringBuilder result = new StringBuilder(); + if (!isEqual) { + result.append("NOT "); + } if (info.lengthExpression != null) { Optional maybeStart = parseIntValue(info.startExpression); Optional maybeLen = parseLengthLiteral(info.lengthExpression, targetString); @@ -571,7 +575,6 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri int start = maybeStart.get(); int len = maybeLen.get(); if (start > 0 && len == targetString.length()) { - StringBuilder result = new StringBuilder(); result.append(info.variableName).append(": \""); for (int i = 1; i < start; i++) { result.append("?"); @@ -586,7 +589,6 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri if (maybeStart.isPresent()) { int start = maybeStart.get(); if (start > 0) { - StringBuilder result = new StringBuilder(); result.append(info.variableName).append(": \""); for (int i = 1; i < start; i++) { result.append("?"); @@ -595,7 +597,8 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri return Optional.of(new ClpExpression(result.toString())); } if (start == -targetString.length()) { - return Optional.of(new ClpExpression(format("%s: \"*%s\"", info.variableName, targetString))); + result.append(format("%s: \"*%s\"", info.variableName, targetString)); + return Optional.of(new ClpExpression(result.toString())); } } } diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java index bfc529cd6275..a4aa0f9bac1e 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java @@ -14,25 +14,29 @@ package com.facebook.presto.plugin.clp; import com.facebook.presto.dispatcher.DispatchManager; -import com.facebook.presto.execution.QueryManager; -import com.facebook.presto.plugin.clp.metadata.ClpMetadataProvider; -import com.facebook.presto.plugin.clp.metadata.ClpMySqlMetadataProvider; +import com.facebook.presto.execution.SqlQueryManager; import com.facebook.presto.plugin.clp.mockdb.ClpMockMetadataDatabase; import com.facebook.presto.plugin.clp.mockdb.table.ColumnMetadataTableRows; +import com.facebook.presto.spi.ConnectorTableLayoutHandle; import com.facebook.presto.spi.QueryId; +import com.facebook.presto.spi.plan.FilterNode; import com.facebook.presto.spi.plan.PlanNode; import com.facebook.presto.spi.plan.PlanNodeId; import com.facebook.presto.spi.plan.TableScanNode; +import com.facebook.presto.spi.relation.RowExpression; +import com.facebook.presto.sql.planner.Plan; import com.facebook.presto.tests.DistributedQueryRunner; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import org.intellij.lang.annotations.Language; -import org.testng.annotations.AfterMethod; -import org.testng.annotations.BeforeMethod; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; import org.testng.annotations.Test; import java.util.Map; import java.util.Optional; +import java.util.concurrent.ExecutionException; +import java.util.regex.Pattern; import static com.facebook.presto.execution.QueryState.RUNNING; import static com.facebook.presto.plugin.clp.ClpQueryRunner.createQueryRunner; @@ -42,7 +46,10 @@ import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Integer; import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.VarString; import static java.lang.String.format; +import static java.util.Objects.requireNonNull; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertNotNull; +import static org.testng.Assert.assertNull; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; @@ -50,17 +57,15 @@ public class TestClpPushDown { private static final String TABLE_NAME = "test_pushdown"; - private static final Long TEST_TS_SECONDS_LOWER_BOUND = 1746003005L; - private static final Long TEST_TS_NANOSECONDS_LOWER_BOUND = 1746003005000000000L; - private static final Long TEST_TS_SECONDS_UPPER_BOUND = 1746003015L; - private static final Long TEST_TS_NANOSECONDS_UPPER_BOUND = 1746003015000000000L; + private static final Pattern BASE_PTR = + Pattern.compile("(?<=base=)\\[B@[0-9a-fA-F]+"); private ClpMockMetadataDatabase mockMetadataDatabase; private DistributedQueryRunner queryRunner; - private QueryManager queryManager; + private SqlQueryManager queryManager; private DispatchManager dispatchManager; - @BeforeMethod + @BeforeClass public void setUp() throws Exception { @@ -72,6 +77,8 @@ public void setUp() ImmutableList.of( "city.Name", "city.Region.id", + "city.Region.population_lowerbound", + "city.Region.population_upperbound", "city.Region.Name", "fare", "isHoliday", @@ -79,17 +86,12 @@ public void setUp() ImmutableList.of( VarString, Integer, + Float, + Float, VarString, Float, Boolean, DateString)))); - ClpConfig config = new ClpConfig() - .setPolymorphicTypeEnabled(true) - .setMetadataDbUrl(mockMetadataDatabase.getUrl()) - .setMetadataDbUser(mockMetadataDatabase.getUsername()) - .setMetadataDbPassword(mockMetadataDatabase.getPassword()) - .setMetadataTablePrefix(mockMetadataDatabase.getTablePrefix()); - ClpMetadataProvider metadataProvider = new ClpMySqlMetadataProvider(config); queryRunner = createQueryRunner( mockMetadataDatabase.getUrl(), mockMetadataDatabase.getUsername(), @@ -97,11 +99,12 @@ public void setUp() mockMetadataDatabase.getTablePrefix(), Optional.of(0), Optional.empty()); - queryManager = queryRunner.getCoordinator().getQueryManager(); + queryManager = (SqlQueryManager) queryRunner.getCoordinator().getQueryManager(); + requireNonNull(queryManager, "queryManager is null"); dispatchManager = queryRunner.getCoordinator().getDispatchManager(); } - @AfterMethod + @AfterClass public void tearDown() throws InterruptedException { @@ -116,66 +119,295 @@ public void tearDown() } } - public void testTimestampComparisons() + @Test + public void testStringMatchPushDown() + { + // Exact match + testPushDown("city.Name = 'hello world'", "city.Name: \"hello world\"", null); + testPushDown("'hello world' = city.Name", "city.Name: \"hello world\"", null); + + // Like predicates that are transformed into substring match + testPushDown("city.Name like 'hello%'", "city.Name: \"hello*\"", null); + testPushDown("city.Name like '%hello'", "city.Name: \"*hello\"", null); + + // Like predicate not pushed down + testPushDown("city.Name like '%hello%'", null, "city.Name like '%hello%'"); + + // Like predicates that are kept in the original forms + testPushDown("city.Name like 'hello_'", "city.Name: \"hello?\"", null); + testPushDown("city.Name like '_hello'", "city.Name: \"?hello\"", null); + testPushDown("city.Name like 'hello_w%'", "city.Name: \"hello?w*\"", null); + testPushDown("city.Name like '%hello_w'", "city.Name: \"*hello?w\"", null); + testPushDown("city.Name like 'hello%world'", "city.Name: \"hello*world\"", null); + testPushDown("city.Name like 'hello%wor%ld'", "city.Name: \"hello*wor*ld\"", null); + } + + @Test + public void testSubStringPushDown() + { + testPushDown("substr(city.Name, 1, 2) = 'he'", "city.Name: \"he*\"", null); + testPushDown("substr(city.Name, 5, 2) = 'he'", "city.Name: \"????he*\"", null); + testPushDown("substr(city.Name, 5) = 'he'", "city.Name: \"????he\"", null); + testPushDown("substr(city.Name, -2) = 'he'", "city.Name: \"*he\"", null); + + // Invalid substring index — not pushed down + testPushDown("substr(city.Name, 1, 5) = 'he'", null, "substr(city.Name, 1, 5) = 'he'"); + testPushDown("substr(city.Name, -5) = 'he'", null, "substr(city.Name, -5) = 'he'"); + } + + @Test + public void testNumericComparisonPushDown() + { + // Numeric comparisons + testPushDown("fare > 0", "fare > 0.0", null); + testPushDown("fare >= 0", "fare >= 0.0", null); + testPushDown("fare < 0", "fare < 0.0", null); + testPushDown("fare <= 0", "fare <= 0.0", null); + testPushDown("fare = 0", "fare: 0.0", null); + testPushDown("fare != 0", "NOT fare: 0.0", null); + testPushDown("fare <> 0", "NOT fare: 0.0", null); + testPushDown("0 < fare", "fare > 0.0", null); + testPushDown("0 <= fare", "fare >= 0.0", null); + testPushDown("0 > fare", "fare < 0.0", null); + testPushDown("0 >= fare", "fare <= 0.0", null); + testPushDown("0 = fare", "fare: 0.0", null); + testPushDown("0 != fare", "NOT fare: 0.0", null); + testPushDown("0 <> fare", "NOT fare: 0.0", null); + } + + @Test + public void testBetweenPushDown() + { + // Normal cases + testPushDown("fare BETWEEN 0 AND 5", "fare >= 0.0 AND fare <= 5.0", null); + testPushDown("fare BETWEEN 5 AND 0", null, null); + + // No push down for non-constant expressions + testPushDown( + "fare BETWEEN city.Region.population_lowerbound AND city.Region.population_upperbound", + null, + "fare >= city.Region.population_lowerbound AND fare <= city.Region.population_upperbound"); + + // If the last two arguments of BETWEEN are not numeric constants, then the CLP connector + // won't push them down. + testPushDown("city.Name BETWEEN 'a' AND 'b'", null, "city.Name >= 'a' AND city.Name <= 'b'"); + } + + @Test + public void testOrPushDown() { - // Test logical binary - testPushDown(format("ts > from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts > %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts >= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts >= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts < from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts < %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts <= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts = from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts != from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("ts <> from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) < ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts > %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) <= ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts >= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) > ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts < %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) >= ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) = ts", TEST_TS_SECONDS_LOWER_BOUND), format("ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) != ts", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); - testPushDown(format("from_unixtime(%s) <> ts", TEST_TS_SECONDS_LOWER_BOUND), format("NOT ts: %s", TEST_TS_NANOSECONDS_LOWER_BOUND)); + // OR conditions with partial push down support + testPushDown("fare > 0 OR city.Name like 'b%'", "(fare > 0.0 OR city.Name: \"b*\")", null); + testPushDown( + "lower(city.Region.Name) = 'hello world' OR city.Region.id != 1", + null, + "(lower(city.Region.Name) = 'hello world' OR city.Region.id != 1)"); + + // Multiple ORs + testPushDown( + "fare > 0 OR city.Name like 'b%' OR lower(city.Region.Name) = 'hello world' OR city.Region.id != 1", + null, + "(fare > 0 OR city.Name like 'b%') OR (lower(city.Region.Name) = 'hello world' OR city.Region.id != 1)"); + testPushDown( + "fare > 0 OR city.Name like 'b%' OR city.Region.id != 1", + "((fare > 0.0 OR city.Name: \"b*\") OR NOT city.Region.id: 1)", + null); + } + + @Test + public void testAndPushDown() + { + // AND conditions with partial/full push down + testPushDown("fare > 0 AND city.Name like 'b%'", "(fare > 0.0 AND city.Name: \"b*\")", null); + + testPushDown("lower(city.Region.Name) = 'hello world' AND city.Region.id != 1", "(NOT city.Region.id: 1)", "lower(city.Region.Name) = 'hello world'"); + + // Multiple ANDs + testPushDown( + "fare > 0 AND city.Name like 'b%' AND lower(city.Region.Name) = 'hello world' AND city.Region.id != 1", + "((fare > 0.0 AND city.Name: \"b*\") AND (NOT city.Region.id: 1))", + "(lower(city.Region.Name) = 'hello world')"); - // Test BETWEEN - testPushDown(format("ts >= from_unixtime(%s) AND ts <= from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND, TEST_TS_SECONDS_UPPER_BOUND), - format("ts >= %s AND ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND, TEST_TS_NANOSECONDS_UPPER_BOUND)); - testPushDown(format("ts BETWEEN from_unixtime(%s) AND from_unixtime(%s)", TEST_TS_SECONDS_LOWER_BOUND, TEST_TS_SECONDS_UPPER_BOUND), - format("ts >= %s AND ts <= %s", TEST_TS_NANOSECONDS_LOWER_BOUND, TEST_TS_NANOSECONDS_UPPER_BOUND)); + testPushDown( + "fare > 0 AND city.Name like '%b%' AND lower(city.Region.Name) = 'hello world' AND city.Region.id != 1", + "((fare > 0.0) AND (NOT city.Region.id: 1))", + "city.Name like '%b%' AND lower(city.Region.Name) = 'hello world'"); } - private void testPushDown(String filter, String expectedPushDown) + @Test + public void testNotPushDown() + { + // NOT and inequality predicates + testPushDown("city.Region.Name NOT LIKE 'hello%'", "NOT city.Region.Name: \"hello*\"", null); + testPushDown("NOT (city.Region.Name LIKE 'hello%')", "NOT city.Region.Name: \"hello*\"", null); + testPushDown("city.Name != 'hello world'", "NOT city.Name: \"hello world\"", null); + testPushDown("city.Name <> 'hello world'", "NOT city.Name: \"hello world\"", null); + testPushDown("NOT (city.Name = 'hello world')", "NOT city.Name: \"hello world\"", null); + testPushDown("fare != 0", "NOT fare: 0.0", null); + testPushDown("fare <> 0", "NOT fare: 0.0", null); + testPushDown("NOT (fare = 0)", "NOT fare: 0.0", null); + + // Multiple NOTs + testPushDown("NOT (NOT fare = 0)", "fare: 0.0", null); + testPushDown("NOT (fare = 0 AND city.Name = 'hello world')", "(NOT fare: 0.0 OR NOT city.Name: \"hello world\")", null); + testPushDown("NOT (fare = 0 OR city.Name = 'hello world')", "(NOT fare: 0.0 AND NOT city.Name: \"hello world\")", null); + } + + @Test + public void testInPushDown() + { + // IN predicate + testPushDown("city.Name IN ('hello world', 'hello world 2')", "(city.Name: \"hello world\" OR city.Name: \"hello world 2\")", null); + } + + @Test + public void testIsNullPushDown() + { + // IS NULL / IS NOT NULL predicates + testPushDown("city.Name IS NULL", "NOT city.Name: *", null); + testPushDown("city.Name IS NOT NULL", "NOT NOT city.Name: *", null); + testPushDown("NOT (city.Name IS NULL)", "NOT NOT city.Name: *", null); + } + + @Test + public void testComplexPushDown() + { + // Complex AND/OR with partial pushdown + testPushDown( + "(fare > 0 OR city.Name like 'b%') AND (lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)", + "((fare > 0.0 OR city.Name: \"b*\"))", + "(lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)"); + + testPushDown( + "city.Region.id = 1 AND (fare > 0 OR city.Name NOT like 'b%') AND (lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)", + "((city.Region.id: 1 AND (fare > 0.0 OR NOT city.Name: \"b*\")))", + "lower(city.Region.Name) = 'hello world' OR city.Name IS NULL"); + } + + @Test + public void testClpWildcardUdf() + { + testPushDown("CLP_WILDCARD_STRING_COLUMN() = 'Beijing'", "*: \"Beijing\"", null); + testPushDown("CLP_WILDCARD_INT_COLUMN() = 1", "*: 1", null); + testPushDown("CLP_WILDCARD_FLOAT_COLUMN() > 0", "* > 0.0", null); + testPushDown("CLP_WILDCARD_BOOL_COLUMN() = true", "*: true", null); + + testPushDown("CLP_WILDCARD_STRING_COLUMN() like 'hello%'", "*: \"hello*\"", null); + testPushDown("substr(CLP_WILDCARD_STRING_COLUMN(), 1, 2) = 'he'", "*: \"he*\"", null); + testPushDown("CLP_WILDCARD_INT_COLUMN() BETWEEN 0 AND 5", "* >= 0 AND * <= 5", null); + testPushDown("CLP_WILDCARD_STRING_COLUMN() IN ('hello world', 'hello world 2')", "(*: \"hello world\" OR *: \"hello world 2\")", null); + + testPushDown("NOT CLP_WILDCARD_FLOAT_COLUMN() > 0", "* <= 0.0", null); + testPushDown( + "CLP_WILDCARD_STRING_COLUMN() = 'Beijing' AND CLP_WILDCARD_INT_COLUMN() = 1 AND city.Region.id = 1", + "((*: \"Beijing\" AND *: 1) AND city.Region.id: 1)", + null); + testPushDown( + "CLP_WILDCARD_STRING_COLUMN() = 'Toronto' OR CLP_WILDCARD_INT_COLUMN() = 2", + "(*: \"Toronto\" OR *: 2)", + null); + testPushDown( + "CLP_WILDCARD_STRING_COLUMN() = 'Shanghai' AND (CLP_WILDCARD_INT_COLUMN() = 3 OR city.Region.id = 5)", + "(*: \"Shanghai\" AND (*: 3 OR city.Region.id: 5))", + null); + } + + private void testPushDown(String filter, String expectedPushDown, String expectedRemaining) { try { - QueryId id = queryRunner.getCoordinator().getDispatchManager().createQueryId(); - @Language("SQL") String sql = format("SELECT * FROM clp.default.test_pushdown WHERE %s LIMIT 1", filter); - dispatchManager.createQuery( - id, - "slug", - 0, - new TestingClpSessionContext(queryRunner.getDefaultSession()), - sql).get(); - long maxDispatchingAndPlanningTime = 60 * 1000L; // 1 minute - long currentWaitingTime = 0L; - while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() && currentWaitingTime < maxDispatchingAndPlanningTime) { - Thread.sleep(1000L); - currentWaitingTime += 1000L; - } - assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); - boolean isPushDownGenerated = false; - for (Map.Entry entry : queryManager.getFullQueryInfo(id).getPlanIdNodeMap().entrySet()) { - if (!(entry.getValue() instanceof TableScanNode) - || (!(((TableScanNode) entry.getValue()).getTable().getLayout().orElse(null) instanceof ClpTableLayoutHandle))) { + // We first execute a query using the original filter and look for the FilterNode (for remaining expression) + // and TableScanNode (for KQL pushdown and split filter pushdown) + QueryId originalQueryId = createAndPlanQuery(filter); + String actualPushDown = null; + RowExpression actualRemainingExpression = null; + Plan originalQueryPlan = queryManager.getQueryPlan(originalQueryId); + for (Map.Entry entry : originalQueryPlan.getPlanIdNodeMap().entrySet()) { + ClpTableLayoutHandle clpTableLayoutHandle = tryGetClpTableLayoutHandleFromFilterNode(entry.getValue()); + if (clpTableLayoutHandle != null && actualRemainingExpression == null) { + actualRemainingExpression = ((FilterNode) entry.getValue()).getPredicate(); continue; } - ClpTableLayoutHandle clpTableLayoutHandle = (ClpTableLayoutHandle) ((TableScanNode) entry.getValue()).getTable().getLayout().orElseThrow(AssertionError::new); - String actualPushDown = clpTableLayoutHandle.getKqlQuery().orElse(null); - assertEquals(actualPushDown, expectedPushDown); - isPushDownGenerated = true; - break; + clpTableLayoutHandle = tryGetClpTableLayoutHandleFromTableScanNode(entry.getValue()); + if (clpTableLayoutHandle != null && actualPushDown == null) { + actualPushDown = clpTableLayoutHandle.getKqlQuery().orElse(null); + } } - assertTrue(isPushDownGenerated); - queryManager.cancelQuery(id); + assertEquals(actualPushDown, expectedPushDown); + if (expectedRemaining != null) { + assertNotNull(actualRemainingExpression); + // Since the remaining expression cannot be simply compared by given String, we have to first convert + // the expectedRemaining to a RowExpression so that we can compare with what we just fetched from the + // plan of the original query. To ensure the translation process is the same, we create another query + // which only contain the remaining expression as the filter, then we look for the FilterNode and get + // the predict as the translated RowExpression of the expectedRemaining. + QueryId remainingFilterQueryId = createAndPlanQuery(expectedRemaining); + Plan remainingFilterPlan = queryManager.getQueryPlan(remainingFilterQueryId); + RowExpression expectedRemainingExpression = null; + for (Map.Entry entry : remainingFilterPlan.getPlanIdNodeMap().entrySet()) { + if (tryGetClpTableLayoutHandleFromFilterNode(entry.getValue()) != null) { + expectedRemainingExpression = ((FilterNode) entry.getValue()).getPredicate(); + break; + } + } + equalsIgnoreBase(actualRemainingExpression, expectedRemainingExpression); + queryManager.cancelQuery(remainingFilterQueryId); + } + queryManager.cancelQuery(originalQueryId); } catch (Exception e) { fail(e.getMessage()); } } + + private QueryId createAndPlanQuery(String filter) + throws ExecutionException, InterruptedException + { + QueryId id = queryRunner.getCoordinator().getDispatchManager().createQueryId(); + @Language("SQL") String sql = format("SELECT * FROM clp.default.test_pushdown WHERE %s LIMIT 1", filter); + dispatchManager.createQuery( + id, + "slug", + 0, + new TestingClpSessionContext(queryRunner.getDefaultSession()), + sql).get(); + long maxDispatchingAndPlanningTime = 60 * 1000L; // 1 minute + long currentWaitingTime = 0L; + while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() && currentWaitingTime < maxDispatchingAndPlanningTime) { + Thread.sleep(1000L); + currentWaitingTime += 1000L; + } + assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); + return id; + } + + private void equalsIgnoreBase(RowExpression actualExpression, RowExpression expectedExpression) + { + if (actualExpression == null) { + assertNull(expectedExpression); + return; + } + String normalizedActualExpressionText = BASE_PTR.matcher(actualExpression.toString()).replaceAll("[B@IGNORED"); + String normalizedExpectedExpressionText = BASE_PTR.matcher(expectedExpression.toString()).replaceAll("[B@IGNORED"); + assertEquals(normalizedActualExpressionText, normalizedExpectedExpressionText); + } + + private ClpTableLayoutHandle tryGetClpTableLayoutHandleFromFilterNode(PlanNode node) + { + if (!(node instanceof FilterNode)) { + return null; + } + return (tryGetClpTableLayoutHandleFromTableScanNode(((FilterNode) node).getSource())); + } + + private ClpTableLayoutHandle tryGetClpTableLayoutHandleFromTableScanNode(PlanNode node) + { + if (!(node instanceof TableScanNode)) { + return null; + } + ConnectorTableLayoutHandle tableLayoutHandle = ((TableScanNode) node).getTable().getLayout().orElse(null); + if (!(tableLayoutHandle instanceof ClpTableLayoutHandle)) { + return null; + } + return (ClpTableLayoutHandle) tableLayoutHandle; + } } From 8cc4449d1e54dfe3a2625f6b61ea81ce4f70859a Mon Sep 17 00:00:00 2001 From: anlowee Date: Tue, 7 Oct 2025 20:43:31 +0000 Subject: [PATCH 4/9] Remove unrelated changes --- .../optimization/ClpFilterToKqlConverter.java | 48 ++++--------------- 1 file changed, 8 insertions(+), 40 deletions(-) diff --git a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java index 442ea82a6acd..f99369b4a48b 100644 --- a/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java +++ b/presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java @@ -16,7 +16,6 @@ import com.facebook.presto.common.function.OperatorType; import com.facebook.presto.common.type.DecimalType; import com.facebook.presto.common.type.RowType; -import com.facebook.presto.common.type.TimestampType; import com.facebook.presto.common.type.Type; import com.facebook.presto.common.type.VarcharType; import com.facebook.presto.plugin.clp.ClpColumnHandle; @@ -58,15 +57,12 @@ import static com.facebook.presto.common.type.IntegerType.INTEGER; import static com.facebook.presto.common.type.RealType.REAL; import static com.facebook.presto.common.type.SmallintType.SMALLINT; -import static com.facebook.presto.common.type.TimestampType.TIMESTAMP; -import static com.facebook.presto.common.type.TimestampType.TIMESTAMP_MICROSECONDS; import static com.facebook.presto.common.type.TinyintType.TINYINT; import static com.facebook.presto.plugin.clp.ClpErrorCode.CLP_PUSHDOWN_UNSUPPORTED_EXPRESSION; import static com.facebook.presto.spi.relation.SpecialFormExpression.Form.AND; import static java.lang.Integer.parseInt; import static java.lang.String.format; import static java.util.Objects.requireNonNull; -import static java.util.concurrent.TimeUnit.SECONDS; /** * A translator to translate Presto {@link RowExpression}s into: @@ -260,10 +256,8 @@ private ClpExpression handleBetween(CallExpression node) } String variable = variableOpt.get(); - Type lowerBoundType = second.getType(); - String lowerBound = tryEnsureNanosecondTimestamp(lowerBoundType, getLiteralString((ConstantExpression) second)); - Type upperBoundType = third.getType(); - String upperBound = tryEnsureNanosecondTimestamp(upperBoundType, getLiteralString((ConstantExpression) third)); + String lowerBound = getLiteralString((ConstantExpression) second); + String upperBound = getLiteralString((ConstantExpression) third); String kql = String.format("%s >= %s AND %s <= %s", variable, lowerBound, variable, upperBound); String metadataSqlQuery = metadataFilterColumns.contains(variable) ? String.format("\"%s\" >= %s AND \"%s\" <= %s", variable, lowerBound, variable, upperBound) @@ -446,7 +440,6 @@ private ClpExpression buildClpExpression( RowExpression originalNode) { String metadataSqlQuery = null; - literalString = tryEnsureNanosecondTimestamp(literalType, literalString); if (operator.equals(EQUAL)) { if (literalType instanceof VarcharType) { return new ClpExpression(format("%s: \"%s\"", variableName, escapeKqlSpecialCharsForStringValue(literalString))); @@ -493,7 +486,7 @@ private Optional tryInterpretSubstringEquality( RowExpression possibleSubstring, RowExpression possibleLiteral) { - if (!operator.equals(EQUAL) && !(operator.equals(NOT_EQUAL))) { + if (!operator.equals(EQUAL)) { return Optional.empty(); } @@ -508,7 +501,7 @@ private Optional tryInterpretSubstringEquality( } String targetString = getLiteralString((ConstantExpression) possibleLiteral); - return interpretSubstringEquality(maybeSubstringCall.get(), targetString, operator.equals(EQUAL)); + return interpretSubstringEquality(maybeSubstringCall.get(), targetString); } /** @@ -561,12 +554,8 @@ private Optional parseSubstringCall(CallExpression callExpression) * @param targetString the literal string being compared to * @return an Optional containing either a ClpExpression with the equivalent KQL query */ - private Optional interpretSubstringEquality(SubstrInfo info, String targetString, boolean isEqual) + private Optional interpretSubstringEquality(SubstrInfo info, String targetString) { - StringBuilder result = new StringBuilder(); - if (!isEqual) { - result.append("NOT "); - } if (info.lengthExpression != null) { Optional maybeStart = parseIntValue(info.startExpression); Optional maybeLen = parseLengthLiteral(info.lengthExpression, targetString); @@ -575,6 +564,7 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri int start = maybeStart.get(); int len = maybeLen.get(); if (start > 0 && len == targetString.length()) { + StringBuilder result = new StringBuilder(); result.append(info.variableName).append(": \""); for (int i = 1; i < start; i++) { result.append("?"); @@ -589,6 +579,7 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri if (maybeStart.isPresent()) { int start = maybeStart.get(); if (start > 0) { + StringBuilder result = new StringBuilder(); result.append(info.variableName).append(": \""); for (int i = 1; i < start; i++) { result.append("?"); @@ -597,8 +588,7 @@ private Optional interpretSubstringEquality(SubstrInfo info, Stri return Optional.of(new ClpExpression(result.toString())); } if (start == -targetString.length()) { - result.append(format("%s: \"*%s\"", info.variableName, targetString)); - return Optional.of(new ClpExpression(result.toString())); + return Optional.of(new ClpExpression(format("%s: \"*%s\"", info.variableName, targetString))); } } } @@ -924,31 +914,9 @@ public static boolean isClpCompatibleNumericType(Type type) || type.equals(TINYINT) || type.equals(DOUBLE) || type.equals(REAL) - || type.equals(TIMESTAMP) - || type.equals(TIMESTAMP_MICROSECONDS) || type instanceof DecimalType; } - private static String tryEnsureNanosecondTimestamp(Type type, String literalString) - { - if (type == TIMESTAMP) { - return ensureNanosecondTimestamp(TIMESTAMP, literalString); - } - else if (type == TIMESTAMP_MICROSECONDS) { - return ensureNanosecondTimestamp(TIMESTAMP_MICROSECONDS, literalString); - } - return literalString; - } - - private static String ensureNanosecondTimestamp(TimestampType type, String literalString) - { - long literalNumber = Long.parseLong(literalString); - long seconds = type.getEpochSecond(literalNumber); - long nanosecondFraction = type.getNanos(literalNumber); - long nanoseconds = SECONDS.toNanos(seconds) + nanosecondFraction; - return Long.toString(nanoseconds); - } - private static class SubstrInfo { String variableName; From d89ca3f1a49ea3d31b9b12751393dfcc89433853 Mon Sep 17 00:00:00 2001 From: anlowee Date: Tue, 7 Oct 2025 20:47:43 +0000 Subject: [PATCH 5/9] Rename --- .../clp/{TestClpPushDown.java => TestClpComputePushDown.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename presto-clp/src/test/java/com/facebook/presto/plugin/clp/{TestClpPushDown.java => TestClpComputePushDown.java} (99%) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java similarity index 99% rename from presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java rename to presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java index a4aa0f9bac1e..d2acdabb189e 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java @@ -54,7 +54,7 @@ import static org.testng.Assert.fail; @Test(singleThreaded = true) -public class TestClpPushDown +public class TestClpComputePushDown { private static final String TABLE_NAME = "test_pushdown"; private static final Pattern BASE_PTR = From ba3674a3f64efde18e7c8fe32ef4d56901c6fdd8 Mon Sep 17 00:00:00 2001 From: anlowee Date: Wed, 8 Oct 2025 19:06:34 +0000 Subject: [PATCH 6/9] Finish --- .../presto/plugin/clp/ClpQueryRunner.java | 117 +++++- .../plugin/clp/TestClpComputePushDown.java | 75 ++++ .../presto/plugin/clp/TestClpFilterToKql.java | 366 ------------------ 3 files changed, 184 insertions(+), 374 deletions(-) delete mode 100644 presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java index dc5b03e4111f..57ddf2561f64 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java @@ -18,12 +18,19 @@ import com.facebook.presto.tests.DistributedQueryRunner; import com.google.common.collect.ImmutableMap; +import java.io.File; +import java.io.FileOutputStream; import java.net.URI; import java.util.Map; import java.util.Optional; +import java.util.UUID; import java.util.function.BiFunction; +import static com.facebook.presto.plugin.clp.ClpConfig.SplitFilterProviderType; import static com.facebook.presto.testing.TestingSession.testSessionBuilder; +import static java.lang.String.format; +import static org.testng.Assert.assertTrue; +import static org.testng.Assert.fail; public class ClpQueryRunner { @@ -43,7 +50,6 @@ public static DistributedQueryRunner createQueryRunner( Optional> externalWorkerLauncher) throws Exception { - log.info("Creating CLP query runner with default session"); return createQueryRunner( createDefaultSession(), metadataDbUrl, @@ -54,6 +60,29 @@ public static DistributedQueryRunner createQueryRunner( externalWorkerLauncher); } + public static DistributedQueryRunner createQueryRunner( + String metadataDbUrl, + String metadataDbUser, + String metadataDbPassword, + String metadataDbTablePrefix, + Optional splitFilterProvider, + Optional splitFilterConfigPath, + Optional workerCount, + Optional> externalWorkerLauncher) + throws Exception + { + return createQueryRunner( + createDefaultSession(), + metadataDbUrl, + metadataDbUser, + metadataDbPassword, + metadataDbTablePrefix, + splitFilterProvider, + splitFilterConfigPath, + workerCount, + externalWorkerLauncher); + } + public static DistributedQueryRunner createQueryRunner( Session session, String metadataDbUrl, @@ -63,25 +92,97 @@ public static DistributedQueryRunner createQueryRunner( Optional workerCount, Optional> externalWorkerLauncher) throws Exception + { + return createQueryRunner( + session, + metadataDbUrl, + metadataDbUser, + metadataDbPassword, + metadataDbTablePrefix, + Optional.empty(), + Optional.empty(), + workerCount, + externalWorkerLauncher); + } + + public static DistributedQueryRunner createQueryRunner( + Session session, + String metadataDbUrl, + String metadataDbUser, + String metadataDbPassword, + String metadataDbTablePrefix, + Optional splitFilterProvider, + Optional splitFilterConfigPath, + Optional workerCount, + Optional> externalWorkerLauncher) + throws Exception { DistributedQueryRunner clpQueryRunner = DistributedQueryRunner.builder(session) - .setNodeCount(workerCount.orElse(DEFAULT_NUM_OF_WORKERS)) - .setExternalWorkerLauncher(externalWorkerLauncher) - .build(); - Map clpProperties = ImmutableMap.builder() + .setNodeCount(workerCount.orElse(DEFAULT_NUM_OF_WORKERS)) + .setExternalWorkerLauncher(externalWorkerLauncher) + .build(); + ImmutableMap.Builder clpProperties = ImmutableMap.builder() .put("clp.metadata-provider-type", "mysql") .put("clp.metadata-db-url", metadataDbUrl) .put("clp.metadata-db-user", metadataDbUser) .put("clp.metadata-db-password", metadataDbPassword) .put("clp.metadata-table-prefix", metadataDbTablePrefix) - .put("clp.split-provider-type", "mysql") - .build(); + .put("clp.split-provider-type", splitFilterProvider.orElse(SplitFilterProviderType.MYSQL.name())); + splitFilterConfigPath.ifPresent(s -> clpProperties.put("clp.split-filter-config", s)); + Map clpPropertiesMap = clpProperties.build(); + log.info("Creating query runner with clp properties:"); + for (Map.Entry entry : clpPropertiesMap.entrySet()) { + log.info("\t%s=%s", entry.getKey(), entry.getValue()); + } clpQueryRunner.installPlugin(new ClpPlugin()); - clpQueryRunner.createCatalog(CLP_CATALOG, CLP_CONNECTOR, clpProperties); + clpQueryRunner.createCatalog(CLP_CATALOG, CLP_CONNECTOR, clpPropertiesMap); return clpQueryRunner; } + /** + * Create a config file with the given literal string of the configuration. + * + * @param configuration the given literal string of the configuration, which will be written into the config file + * @return the string of the config file path + */ + public static String createConfigFile(String configuration) + { + UUID uuid = UUID.randomUUID(); + File file = new File(format("/tmp/config-%s", uuid)); + try { + boolean fileCreated = file.createNewFile(); + assertTrue(fileCreated); + } + catch (Exception e) { + fail(e.getMessage()); + } + + if (!file.exists() || !file.canWrite()) { + fail("Cannot create split filter config file"); + } + try (FileOutputStream fos = new FileOutputStream(file)) { + fos.write(configuration.getBytes()); + } + catch (Exception e) { + fail(e.getMessage()); + } + return file.getAbsolutePath(); + } + + /** + * Delete a config file by the given path. + * + * @param configFilePath the path of the config file + */ + public static void deleteConfigFile(String configFilePath) + { + File splitFilterConfigFile = new File(configFilePath); + if (splitFilterConfigFile.exists()) { + assertTrue(splitFilterConfigFile.delete()); + } + } + /** * Creates a default mock session for query use. * diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java index d2acdabb189e..1cc4fab81c59 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java @@ -39,7 +39,9 @@ import java.util.regex.Pattern; import static com.facebook.presto.execution.QueryState.RUNNING; +import static com.facebook.presto.plugin.clp.ClpQueryRunner.createConfigFile; import static com.facebook.presto.plugin.clp.ClpQueryRunner.createQueryRunner; +import static com.facebook.presto.plugin.clp.ClpQueryRunner.deleteConfigFile; import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Boolean; import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.DateString; import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Float; @@ -64,11 +66,13 @@ public class TestClpComputePushDown private DistributedQueryRunner queryRunner; private SqlQueryManager queryManager; private DispatchManager dispatchManager; + private String splitFilterConfigFilePath; @BeforeClass public void setUp() throws Exception { + // Set up metadata database mockMetadataDatabase = ClpMockMetadataDatabase .builder() .build(); @@ -92,11 +96,29 @@ public void setUp() Float, Boolean, DateString)))); + // Set up split filter config + String splitFilterConfigJsonString = "{\n" + + " \"clp\": [\n" + + " {\n" + + " \"columnName\": \"fare\",\n" + + " \"customOptions\": {\n" + + " \"rangeMapping\": {\n" + + " \"lowerBound\": \"fare_lb\",\n" + + " \"upperBound\": \"fare_ub\"\n" + + " }\n" + + " },\n" + + " \"required\": false\n" + + " }\n" + + " ]\n" + + "}"; + splitFilterConfigFilePath = createConfigFile(splitFilterConfigJsonString); queryRunner = createQueryRunner( mockMetadataDatabase.getUrl(), mockMetadataDatabase.getUsername(), mockMetadataDatabase.getPassword(), mockMetadataDatabase.getTablePrefix(), + Optional.empty(), + Optional.of(splitFilterConfigFilePath), Optional.of(0), Optional.empty()); queryManager = (SqlQueryManager) queryRunner.getCoordinator().getQueryManager(); @@ -117,6 +139,7 @@ public void tearDown() if (null != mockMetadataDatabase) { mockMetadataDatabase.teardown(); } + deleteConfigFile(splitFilterConfigFilePath); } @Test @@ -284,6 +307,43 @@ public void testComplexPushDown() "lower(city.Region.Name) = 'hello world' OR city.Name IS NULL"); } + @Test + public void testSplitFilterPushDown() + { + // Normal case + testPushDown( + "(fare > 0 AND city.Name like 'b%')", + "(fare > 0.0 AND city.Name: \"b*\")", + null, + "(fare_ub > 0.0)"); + + // With BETWEEN + testPushDown( + "((fare BETWEEN 0 AND 5) AND city.Name like 'b%')", + "(fare >= 0.0 AND fare <= 5.0 AND city.Name: \"b*\")", + null, + "(fare_ub >= 0.0 AND fare_lb <= 5.0)"); + + // The cases of that the metadata filter column exist but cannot be push down + testPushDown( + "(fare > 0 OR city.Name like 'b%')", + "(fare > 0.0 OR city.Name: \"b*\")", + null, + null); + testPushDown( + "(fare > 0 AND city.Name like 'b%') OR city.Region.Id = 1", + "((city.Region.id: 1 OR fare > 0.0) AND (city.Region.id: 1 OR city.Name: \"b*\"))", + null, + null); + + // Complicated case + testPushDown( + "fare = 0 AND (city.Name like 'b%' OR city.Region.Id = 1)", + "(fare: 0.0 AND (city.Name: \"b*\" OR city.Region.id: 1))", + null, + "((fare_lb <= 0.0 AND fare_ub >= 0.0))"); + } + @Test public void testClpWildcardUdf() { @@ -313,12 +373,23 @@ public void testClpWildcardUdf() } private void testPushDown(String filter, String expectedPushDown, String expectedRemaining) + { + testPushDown(filter, expectedPushDown, expectedRemaining, true, null); + } + + private void testPushDown(String filter, String expectedPushDown, String expectedRemaining, String expectedSplitFilterPushDown) + { + testPushDown(filter, expectedPushDown, expectedRemaining, false, expectedSplitFilterPushDown); + } + + private void testPushDown(String filter, String expectedPushDown, String expectedRemaining, boolean ignoreSplitFilterPushDown, String expectedSplitFilterPushDown) { try { // We first execute a query using the original filter and look for the FilterNode (for remaining expression) // and TableScanNode (for KQL pushdown and split filter pushdown) QueryId originalQueryId = createAndPlanQuery(filter); String actualPushDown = null; + String actualSplitFilterPushDown = null; RowExpression actualRemainingExpression = null; Plan originalQueryPlan = queryManager.getQueryPlan(originalQueryId); for (Map.Entry entry : originalQueryPlan.getPlanIdNodeMap().entrySet()) { @@ -330,9 +401,13 @@ private void testPushDown(String filter, String expectedPushDown, String expecte clpTableLayoutHandle = tryGetClpTableLayoutHandleFromTableScanNode(entry.getValue()); if (clpTableLayoutHandle != null && actualPushDown == null) { actualPushDown = clpTableLayoutHandle.getKqlQuery().orElse(null); + actualSplitFilterPushDown = clpTableLayoutHandle.getMetadataSql().orElse(null); } } assertEquals(actualPushDown, expectedPushDown); + if (!ignoreSplitFilterPushDown) { + assertEquals(actualSplitFilterPushDown, expectedSplitFilterPushDown); + } if (expectedRemaining != null) { assertNotNull(actualRemainingExpression); // Since the remaining expression cannot be simply compared by given String, we have to first convert diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java deleted file mode 100644 index 413c9cd773a6..000000000000 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java +++ /dev/null @@ -1,366 +0,0 @@ -/* - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package com.facebook.presto.plugin.clp; - -import com.facebook.presto.plugin.clp.optimization.ClpFilterToKqlConverter; -import com.facebook.presto.spi.ColumnHandle; -import com.facebook.presto.spi.relation.RowExpression; -import com.facebook.presto.spi.relation.VariableReferenceExpression; -import com.google.common.collect.ImmutableSet; -import org.testng.annotations.Test; - -import java.util.HashMap; -import java.util.Map; -import java.util.Optional; -import java.util.Set; - -import static org.testng.Assert.assertEquals; -import static org.testng.Assert.assertFalse; -import static org.testng.Assert.assertTrue; - -@Test -public class TestClpFilterToKql - extends TestClpQueryBase -{ - @Test - public void testStringMatchPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // Exact match - testPushDown(sessionHolder, "city.Name = 'hello world'", "city.Name: \"hello world\"", null); - testPushDown(sessionHolder, "'hello world' = city.Name", "city.Name: \"hello world\"", null); - - // Like predicates that are transformed into substring match - testPushDown(sessionHolder, "city.Name like 'hello%'", "city.Name: \"hello*\"", null); - testPushDown(sessionHolder, "city.Name like '%hello'", "city.Name: \"*hello\"", null); - - // Like predicate not pushed down - testPushDown(sessionHolder, "city.Name like '%hello%'", null, "city.Name like '%hello%'"); - - // Like predicates that are kept in the original forms - testPushDown(sessionHolder, "city.Name like 'hello_'", "city.Name: \"hello?\"", null); - testPushDown(sessionHolder, "city.Name like '_hello'", "city.Name: \"?hello\"", null); - testPushDown(sessionHolder, "city.Name like 'hello_w%'", "city.Name: \"hello?w*\"", null); - testPushDown(sessionHolder, "city.Name like '%hello_w'", "city.Name: \"*hello?w\"", null); - testPushDown(sessionHolder, "city.Name like 'hello%world'", "city.Name: \"hello*world\"", null); - testPushDown(sessionHolder, "city.Name like 'hello%wor%ld'", "city.Name: \"hello*wor*ld\"", null); - } - - @Test - public void testSubStringPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - testPushDown(sessionHolder, "substr(city.Name, 1, 2) = 'he'", "city.Name: \"he*\"", null); - testPushDown(sessionHolder, "substr(city.Name, 5, 2) = 'he'", "city.Name: \"????he*\"", null); - testPushDown(sessionHolder, "substr(city.Name, 5) = 'he'", "city.Name: \"????he\"", null); - testPushDown(sessionHolder, "substr(city.Name, -2) = 'he'", "city.Name: \"*he\"", null); - - // Invalid substring index — not pushed down - testPushDown(sessionHolder, "substr(city.Name, 1, 5) = 'he'", null, "substr(city.Name, 1, 5) = 'he'"); - testPushDown(sessionHolder, "substr(city.Name, -5) = 'he'", null, "substr(city.Name, -5) = 'he'"); - } - - @Test - public void testNumericComparisonPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // Numeric comparisons - testPushDown(sessionHolder, "fare > 0", "fare > 0", null); - testPushDown(sessionHolder, "fare >= 0", "fare >= 0", null); - testPushDown(sessionHolder, "fare < 0", "fare < 0", null); - testPushDown(sessionHolder, "fare <= 0", "fare <= 0", null); - testPushDown(sessionHolder, "fare = 0", "fare: 0", null); - testPushDown(sessionHolder, "fare != 0", "NOT fare: 0", null); - testPushDown(sessionHolder, "fare <> 0", "NOT fare: 0", null); - testPushDown(sessionHolder, "0 < fare", "fare > 0", null); - testPushDown(sessionHolder, "0 <= fare", "fare >= 0", null); - testPushDown(sessionHolder, "0 > fare", "fare < 0", null); - testPushDown(sessionHolder, "0 >= fare", "fare <= 0", null); - testPushDown(sessionHolder, "0 = fare", "fare: 0", null); - testPushDown(sessionHolder, "0 != fare", "NOT fare: 0", null); - testPushDown(sessionHolder, "0 <> fare", "NOT fare: 0", null); - } - - @Test - public void testBetweenPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // Normal cases - testPushDown(sessionHolder, "fare BETWEEN 0 AND 5", "fare >= 0 AND fare <= 5", null); - testPushDown(sessionHolder, "fare BETWEEN 5 AND 0", "fare >= 5 AND fare <= 0", null); - - // No push down for non-constant expressions - testPushDown( - sessionHolder, - "fare BETWEEN (city.Region.Id - 5) AND (city.Region.Id + 5)", - null, - "fare BETWEEN (city.Region.Id - 5) AND (city.Region.Id + 5)"); - - // If the last two arguments of BETWEEN are not numeric constants, then the CLP connector - // won't push them down. - testPushDown(sessionHolder, "city.Name BETWEEN 'a' AND 'b'", null, "city.Name BETWEEN 'a' AND 'b'"); - } - - @Test - public void testOrPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // OR conditions with partial push down support - testPushDown(sessionHolder, "fare > 0 OR city.Name like 'b%'", "(fare > 0 OR city.Name: \"b*\")", null); - testPushDown( - sessionHolder, - "lower(city.Region.Name) = 'hello world' OR city.Region.Id != 1", - null, - "(lower(city.Region.Name) = 'hello world' OR city.Region.Id != 1)"); - - // Multiple ORs - testPushDown( - sessionHolder, - "fare > 0 OR city.Name like 'b%' OR lower(city.Region.Name) = 'hello world' OR city.Region.Id != 1", - null, - "fare > 0 OR city.Name like 'b%' OR lower(city.Region.Name) = 'hello world' OR city.Region.Id != 1"); - testPushDown( - sessionHolder, - "fare > 0 OR city.Name like 'b%' OR city.Region.Id != 1", - "((fare > 0 OR city.Name: \"b*\") OR NOT city.Region.Id: 1)", - null); - } - - @Test - public void testAndPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // AND conditions with partial/full push down - testPushDown(sessionHolder, "fare > 0 AND city.Name like 'b%'", "(fare > 0 AND city.Name: \"b*\")", null); - - testPushDown(sessionHolder, "lower(city.Region.Name) = 'hello world' AND city.Region.Id != 1", "(NOT city.Region.Id: 1)", "lower(city.Region.Name) = 'hello world'"); - - // Multiple ANDs - testPushDown( - sessionHolder, - "fare > 0 AND city.Name like 'b%' AND lower(city.Region.Name) = 'hello world' AND city.Region.Id != 1", - "(((fare > 0 AND city.Name: \"b*\")) AND NOT city.Region.Id: 1)", - "(lower(city.Region.Name) = 'hello world')"); - - testPushDown( - sessionHolder, - "fare > 0 AND city.Name like '%b%' AND lower(city.Region.Name) = 'hello world' AND city.Region.Id != 1", - "(((fare > 0)) AND NOT city.Region.Id: 1)", - "city.Name like '%b%' AND lower(city.Region.Name) = 'hello world'"); - } - - @Test - public void testNotPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // NOT and inequality predicates - testPushDown(sessionHolder, "city.Region.Name NOT LIKE 'hello%'", "NOT city.Region.Name: \"hello*\"", null); - testPushDown(sessionHolder, "NOT (city.Region.Name LIKE 'hello%')", "NOT city.Region.Name: \"hello*\"", null); - testPushDown(sessionHolder, "city.Name != 'hello world'", "NOT city.Name: \"hello world\"", null); - testPushDown(sessionHolder, "city.Name <> 'hello world'", "NOT city.Name: \"hello world\"", null); - testPushDown(sessionHolder, "NOT (city.Name = 'hello world')", "NOT city.Name: \"hello world\"", null); - testPushDown(sessionHolder, "fare != 0", "NOT fare: 0", null); - testPushDown(sessionHolder, "fare <> 0", "NOT fare: 0", null); - testPushDown(sessionHolder, "NOT (fare = 0)", "NOT fare: 0", null); - - // Multiple NOTs - testPushDown(sessionHolder, "NOT (NOT fare = 0)", "NOT NOT fare: 0", null); - testPushDown(sessionHolder, "NOT (fare = 0 AND city.Name = 'hello world')", "NOT (fare: 0 AND city.Name: \"hello world\")", null); - testPushDown(sessionHolder, "NOT (fare = 0 OR city.Name = 'hello world')", "NOT (fare: 0 OR city.Name: \"hello world\")", null); - } - - @Test - public void testInPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // IN predicate - testPushDown(sessionHolder, "city.Name IN ('hello world', 'hello world 2')", "(city.Name: \"hello world\" OR city.Name: \"hello world 2\")", null); - } - - @Test - public void testIsNullPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // IS NULL / IS NOT NULL predicates - testPushDown(sessionHolder, "city.Name IS NULL", "NOT city.Name: *", null); - testPushDown(sessionHolder, "city.Name IS NOT NULL", "NOT NOT city.Name: *", null); - testPushDown(sessionHolder, "NOT (city.Name IS NULL)", "NOT NOT city.Name: *", null); - } - - @Test - public void testComplexPushDown() - { - SessionHolder sessionHolder = new SessionHolder(); - - // Complex AND/OR with partial pushdown - testPushDown( - sessionHolder, - "(fare > 0 OR city.Name like 'b%') AND (lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)", - "((fare > 0 OR city.Name: \"b*\"))", - "(lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)"); - - testPushDown( - sessionHolder, - "city.Region.Id = 1 AND (fare > 0 OR city.Name NOT like 'b%') AND (lower(city.Region.Name) = 'hello world' OR city.Name IS NULL)", - "((city.Region.Id: 1 AND (fare > 0 OR NOT city.Name: \"b*\")))", - "lower(city.Region.Name) = 'hello world' OR city.Name IS NULL"); - } - - @Test - public void testMetadataSqlGeneration() - { - SessionHolder sessionHolder = new SessionHolder(); - Set testMetadataFilterColumns = ImmutableSet.of("fare"); - - // Normal case - testPushDown( - sessionHolder, - "(fare > 0 AND city.Name like 'b%')", - "(fare > 0 AND city.Name: \"b*\")", - "(\"fare\" > 0)", - testMetadataFilterColumns); - - // With BETWEEN - testPushDown( - sessionHolder, - "((fare BETWEEN 0 AND 5) AND city.Name like 'b%')", - "(fare >= 0 AND fare <= 5 AND city.Name: \"b*\")", - "(\"fare\" >= 0 AND \"fare\" <= 5)", - testMetadataFilterColumns); - - // The cases of that the metadata filter column exist but cannot be push down - testPushDown( - sessionHolder, - "(fare > 0 OR city.Name like 'b%')", - "(fare > 0 OR city.Name: \"b*\")", - null, - testMetadataFilterColumns); - testPushDown( - sessionHolder, - "(fare > 0 AND city.Name like 'b%') OR city.Region.Id = 1", - "((fare > 0 AND city.Name: \"b*\") OR city.Region.Id: 1)", - null, - testMetadataFilterColumns); - - // Complicated case - testPushDown( - sessionHolder, - "fare = 0 AND (city.Name like 'b%' OR city.Region.Id = 1)", - "(fare: 0 AND (city.Name: \"b*\" OR city.Region.Id: 1))", - "(\"fare\" = 0)", - testMetadataFilterColumns); - } - - @Test - public void testClpWildcardUdf() - { - SessionHolder sessionHolder = new SessionHolder(); - - testPushDown(sessionHolder, "CLP_WILDCARD_STRING_COLUMN() = 'Beijing'", "*: \"Beijing\"", null); - testPushDown(sessionHolder, "CLP_WILDCARD_INT_COLUMN() = 1", "*: 1", null); - testPushDown(sessionHolder, "CLP_WILDCARD_FLOAT_COLUMN() > 0", "* > 0", null); - testPushDown(sessionHolder, "CLP_WILDCARD_BOOL_COLUMN() = true", "*: true", null); - - testPushDown(sessionHolder, "CLP_WILDCARD_STRING_COLUMN() like 'hello%'", "*: \"hello*\"", null); - testPushDown(sessionHolder, "substr(CLP_WILDCARD_STRING_COLUMN(), 1, 2) = 'he'", "*: \"he*\"", null); - testPushDown(sessionHolder, "CLP_WILDCARD_INT_COLUMN() BETWEEN 0 AND 5", "* >= 0 AND * <= 5", null); - testPushDown(sessionHolder, "CLP_WILDCARD_STRING_COLUMN() IN ('hello world', 'hello world 2')", "(*: \"hello world\" OR *: \"hello world 2\")", null); - - testPushDown(sessionHolder, "NOT CLP_WILDCARD_FLOAT_COLUMN() > 0", "NOT * > 0", null); - testPushDown( - sessionHolder, - "CLP_WILDCARD_STRING_COLUMN() = 'Beijing' AND CLP_WILDCARD_INT_COLUMN() = 1 AND city.Region.Id = 1", - "((*: \"Beijing\" AND *: 1) AND city.Region.Id: 1)", - null); - testPushDown( - sessionHolder, - "CLP_WILDCARD_STRING_COLUMN() = 'Toronto' OR CLP_WILDCARD_INT_COLUMN() = 2", - "(*: \"Toronto\" OR *: 2)", - null); - testPushDown( - sessionHolder, - "CLP_WILDCARD_STRING_COLUMN() = 'Shanghai' AND (CLP_WILDCARD_INT_COLUMN() = 3 OR city.Region.Id = 5)", - "(*: \"Shanghai\" AND (*: 3 OR city.Region.Id: 5))", - null); - } - - private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedRemaining) - { - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, ImmutableSet.of()); - testFilter(clpExpression, expectedKql, expectedRemaining, sessionHolder); - } - - private void testPushDown(SessionHolder sessionHolder, String sql, String expectedKql, String expectedMetadataSqlQuery, Set metadataFilterColumns) - { - ClpExpression clpExpression = tryPushDown(sql, sessionHolder, metadataFilterColumns); - testFilter(clpExpression, expectedKql, null, sessionHolder); - if (expectedMetadataSqlQuery != null) { - assertTrue(clpExpression.getMetadataSqlQuery().isPresent()); - assertEquals(clpExpression.getMetadataSqlQuery().get(), expectedMetadataSqlQuery); - } - else { - assertFalse(clpExpression.getMetadataSqlQuery().isPresent()); - } - } - - private ClpExpression tryPushDown( - String sqlExpression, - SessionHolder sessionHolder, - Set metadataFilterColumns) - { - RowExpression pushDownExpression = getRowExpression(sqlExpression, sessionHolder); - Map assignments = new HashMap<>(variableToColumnHandleMap); - return pushDownExpression.accept( - new ClpFilterToKqlConverter( - standardFunctionResolution, - functionAndTypeManager, - assignments, - metadataFilterColumns), - null); - } - - private void testFilter( - ClpExpression clpExpression, - String expectedKqlExpression, - String expectedRemainingExpression, - SessionHolder sessionHolder) - { - Optional kqlExpression = clpExpression.getPushDownExpression(); - Optional remainingExpression = clpExpression.getRemainingExpression(); - if (expectedKqlExpression != null) { - assertTrue(kqlExpression.isPresent()); - assertEquals(kqlExpression.get(), expectedKqlExpression); - } - else { - assertFalse(kqlExpression.isPresent()); - } - - if (expectedRemainingExpression != null) { - assertTrue(remainingExpression.isPresent()); - assertEquals(remainingExpression.get(), getRowExpression(expectedRemainingExpression, sessionHolder)); - } - else { - assertFalse(remainingExpression.isPresent()); - } - } -} From a230472798cf8a4bf276a4caafabe7647feca602 Mon Sep 17 00:00:00 2001 From: anlowee Date: Wed, 8 Oct 2025 19:32:09 +0000 Subject: [PATCH 7/9] Address coderabbitai comments --- .../com/facebook/presto/plugin/clp/ClpQueryRunner.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java index 57ddf2561f64..88910857a5f1 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java @@ -27,8 +27,10 @@ import java.util.function.BiFunction; import static com.facebook.presto.plugin.clp.ClpConfig.SplitFilterProviderType; +import static com.facebook.presto.plugin.clp.ClpConfig.SplitProviderType; import static com.facebook.presto.testing.TestingSession.testSessionBuilder; import static java.lang.String.format; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.testng.Assert.assertTrue; import static org.testng.Assert.fail; @@ -127,7 +129,8 @@ public static DistributedQueryRunner createQueryRunner( .put("clp.metadata-db-user", metadataDbUser) .put("clp.metadata-db-password", metadataDbPassword) .put("clp.metadata-table-prefix", metadataDbTablePrefix) - .put("clp.split-provider-type", splitFilterProvider.orElse(SplitFilterProviderType.MYSQL.name())); + .put("clp.split-provider-type", SplitProviderType.MYSQL.name()) + .put("clp.split-filter-provider-type", splitFilterProvider.orElse(SplitFilterProviderType.MYSQL.name())); splitFilterConfigPath.ifPresent(s -> clpProperties.put("clp.split-filter-config", s)); Map clpPropertiesMap = clpProperties.build(); @@ -149,7 +152,7 @@ public static DistributedQueryRunner createQueryRunner( public static String createConfigFile(String configuration) { UUID uuid = UUID.randomUUID(); - File file = new File(format("/tmp/config-%s", uuid)); + File file = new File(System.getProperty("java.io.tmpdir"), format("config-%s", uuid)); try { boolean fileCreated = file.createNewFile(); assertTrue(fileCreated); @@ -162,7 +165,7 @@ public static String createConfigFile(String configuration) fail("Cannot create split filter config file"); } try (FileOutputStream fos = new FileOutputStream(file)) { - fos.write(configuration.getBytes()); + fos.write(configuration.getBytes(UTF_8)); } catch (Exception e) { fail(e.getMessage()); From c9d78d1f02e6b8b88c782e6e04406584cb11dd9c Mon Sep 17 00:00:00 2001 From: anlowee Date: Wed, 8 Oct 2025 19:37:18 +0000 Subject: [PATCH 8/9] Address coderabbitai comments --- .../plugin/clp/TestClpComputePushDown.java | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java index d2acdabb189e..16f0e3bd69d7 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java @@ -108,6 +108,7 @@ public void setUp() public void tearDown() throws InterruptedException { + queryRunner.cancelAllQueries(); long maxCleanUpTime = 5 * 1000L; // 5 seconds long currentCleanUpTime = 0L; while (!queryManager.getQueries().isEmpty() && currentCleanUpTime < maxCleanUpTime) { @@ -314,10 +315,12 @@ public void testClpWildcardUdf() private void testPushDown(String filter, String expectedPushDown, String expectedRemaining) { + QueryId originalQueryId = null; + QueryId remainingFilterQueryId = null; try { // We first execute a query using the original filter and look for the FilterNode (for remaining expression) // and TableScanNode (for KQL pushdown and split filter pushdown) - QueryId originalQueryId = createAndPlanQuery(filter); + originalQueryId = createAndPlanQuery(filter); String actualPushDown = null; RowExpression actualRemainingExpression = null; Plan originalQueryPlan = queryManager.getQueryPlan(originalQueryId); @@ -340,7 +343,7 @@ private void testPushDown(String filter, String expectedPushDown, String expecte // plan of the original query. To ensure the translation process is the same, we create another query // which only contain the remaining expression as the filter, then we look for the FilterNode and get // the predict as the translated RowExpression of the expectedRemaining. - QueryId remainingFilterQueryId = createAndPlanQuery(expectedRemaining); + remainingFilterQueryId = createAndPlanQuery(expectedRemaining); Plan remainingFilterPlan = queryManager.getQueryPlan(remainingFilterQueryId); RowExpression expectedRemainingExpression = null; for (Map.Entry entry : remainingFilterPlan.getPlanIdNodeMap().entrySet()) { @@ -350,13 +353,19 @@ private void testPushDown(String filter, String expectedPushDown, String expecte } } equalsIgnoreBase(actualRemainingExpression, expectedRemainingExpression); - queryManager.cancelQuery(remainingFilterQueryId); } - queryManager.cancelQuery(originalQueryId); } catch (Exception e) { fail(e.getMessage()); } + finally { + if (originalQueryId != null) { + queryManager.cancelQuery(originalQueryId); + } + if (remainingFilterQueryId != null) { + queryManager.cancelQuery(remainingFilterQueryId); + } + } } private QueryId createAndPlanQuery(String filter) From 27b83ade90f7d2085ec711d4f747d5f8bd40b32d Mon Sep 17 00:00:00 2001 From: anlowee Date: Wed, 8 Oct 2025 19:45:04 +0000 Subject: [PATCH 9/9] Address coderabbitai comments --- .../com/facebook/presto/plugin/clp/TestClpComputePushDown.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java index 16f0e3bd69d7..d034fee63655 100644 --- a/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java +++ b/presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java @@ -354,6 +354,9 @@ private void testPushDown(String filter, String expectedPushDown, String expecte } equalsIgnoreBase(actualRemainingExpression, expectedRemainingExpression); } + else { + assertNull(actualRemainingExpression); + } } catch (Exception e) { fail(e.getMessage());