refactor: Refactor the split filter pushdown unit tests. #79
Conversation
… TestClpFilterToKql to TestClpPushDown
anlowee
force-pushed
the
xwei/add-splitfilter-runner
branch
from
914d2b0 to
ba3674a
Compare
WalkthroughAdded a test-scoped dependency on Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant TestClass as TestClpComputePushDown
participant QR as ClpQueryRunner
participant DQR as DistributedQueryRunner
participant Cat as Catalog
participant SF as SplitFilterConfig
Tester->>TestClass: run suite
TestClass->>QR: createQueryRunner(metadataDb..., splitFilterProvider?, splitFilterConfigPath?, ...)
alt split-filter config provided
QR->>SF: createConfigFile / provide config path
end
QR->>Cat: assemble clpProperties map (incl. split-provider-type, split-filter-config?)
Cat-->>QR: catalog created
QR-->>TestClass: DistributedQueryRunner ready
TestClass->>DQR: execute queries / request plans
DQR-->>TestClass: plans (KQL/metadata + remaining filters)
TestClass->>SF: deleteConfigFile (cleanup)
sequenceDiagram
autonumber
actor Tester
participant DQR as DistributedQueryRunner
participant Planner as Planner
participant CLP as CLP Connector
participant SFP as SplitFilterProvider
Tester->>DQR: plan query
DQR->>Planner: build logical plan
Planner->>CLP: apply connector pushdown rules
CLP->>SFP: request split-filter (config/provider)
SFP-->>CLP: split-filter expression
CLP-->>Planner: KQL/metadata SQL + remaining filter
Planner-->>Tester: final plan
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
presto-clp/pom.xml(1 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java(3 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java(1 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java(0 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java(1 hunks)
💤 Files with no reviewable changes (1)
- presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
🧰 Additional context used
🧬 Code graph analysis (3)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java (1)
presto-main-base/src/main/java/com/facebook/presto/Session.java (1)
Session(76-950)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (1)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java (1)
ClpConfig(21-204)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (5)
presto-main-base/src/main/java/com/facebook/presto/dispatcher/DispatchManager.java (1)
DispatchManager(70-472)presto-clp/src/test/java/com/facebook/presto/plugin/clp/mockdb/ClpMockMetadataDatabase.java (1)
ClpMockMetadataDatabase(49-288)presto-clp/src/test/java/com/facebook/presto/plugin/clp/mockdb/table/ColumnMetadataTableRows.java (1)
ColumnMetadataTableRows(30-63)presto-tests/src/main/java/com/facebook/presto/tests/DistributedQueryRunner.java (1)
DistributedQueryRunner(108-1224)presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (1)
ClpQueryRunner(35-202)
🔇 Additional comments (9)
presto-clp/pom.xml (1)
108-112: LGTM!The test-scoped dependency on
presto-mainappropriately supports the new test infrastructure introduced in this PR, including access toDispatchManager,SqlQueryManager, and related query execution components used byTestClpComputePushDown.presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java (3)
40-43: LGTM!The constructor correctly validates the session parameter with a null check.
52-61: LGTM!Providing default values ("clp" and "default") for catalog and schema when absent from the session ensures consistent behavior in test contexts.
129-137: LGTM!The conversion from
ConnectorId-keyed properties to catalog-name-keyed properties is correctly implemented using Guava'sImmutableMap.builder().presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (2)
71-127: LGTM!The setUp method correctly initializes the mock database, split filter configuration, and query runner. The use of
Optional.of(0)for worker count appropriately creates a coordinator-only setup for testing pushdown logic.
437-456: Verify timeout handling and improve diagnostics.The method waits up to 60 seconds for the query to reach RUNNING state. If this timeout is exceeded, the assertion at line 454 will fail, but the query may remain active and uncancelled, potentially affecting subsequent tests.
Consider these improvements:
- Cancel the query if the timeout is exceeded before failing the assertion.
- Provide a more descriptive failure message that includes the actual query state and query ID.
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); + if (currentWaitingTime >= maxDispatchingAndPlanningTime) { + queryManager.cancelQuery(id); + fail(format("Query %s did not reach RUNNING state within timeout. Final state: %s", + id, dispatchManager.getQueryInfo(id).getState())); + } return id; }presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (3)
63-84: LGTM!The new overload correctly delegates to the session-based variant with default session, maintaining a clean API for callers who don't need custom session configuration.
95-106: LGTM!The modification maintains backward compatibility by delegating to the comprehensive overload with empty Optional values for split filter parameters.
133-137: LGTM!The logging enhancement provides useful visibility into the finalized CLP properties before catalog creation, which aids in debugging test configurations.
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (1)
181-187: TestNG coupling remains (previously flagged).The
deleteConfigFilemethod still usesassertTruefrom TestNG, coupling this utility to the test framework. This was previously flagged as a nitpick-level concern in past reviews but has not been addressed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (2)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/ClpConfig.java (1)
ClpConfig(21-204)presto-tests/src/main/java/com/facebook/presto/tests/DistributedQueryRunner.java (1)
DistributedQueryRunner(108-1224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: test (17.0.13, :presto-main-base)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-aggregation-queries)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-main-base)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-plan-determinism)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-local-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-tpch-distributed-queries)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (2)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (2)
21-36: Imports support new config file functionality.The imports are appropriate for the new config file handling methods. The UTF_8 charset import indicates the charset issue from previous reviews has been addressed.
Note: TestNG assertion imports (assertTrue, fail) remain, which couples this utility to the test framework. This was previously flagged as a nitpick-level concern.
65-108: Overload design provides good backward compatibility.The new overloads that accept
splitFilterProviderandsplitFilterConfigPathparameters are well-designed. The existing method now forwardsOptional.empty()for these parameters, maintaining backward compatibility while enabling split-filter configuration for new test cases.
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java
Show resolved
Hide resolved
presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (1)
433-434: Complete the exception handling improvement.The previous review's suggestion to improve exception context has been partially addressed (QueryId declarations moved, finally block added). However,
fail(e.getMessage())still loses the stack trace and filter context.Apply this diff to preserve full exception context:
- catch (Exception e) { - fail(e.getMessage()); - } + catch (Exception e) { + fail(format("Test failed for filter '%s': %s", filter, e.getMessage()), e); + }Based on past review comments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (4)
presto-main-base/src/main/java/com/facebook/presto/dispatcher/DispatchManager.java (1)
DispatchManager(70-472)presto-clp/src/test/java/com/facebook/presto/plugin/clp/mockdb/ClpMockMetadataDatabase.java (1)
ClpMockMetadataDatabase(49-288)presto-tests/src/main/java/com/facebook/presto/tests/DistributedQueryRunner.java (1)
DistributedQueryRunner(108-1224)presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (1)
ClpQueryRunner(37-205)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (3)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (3)
129-144: Previous feedback addressed.The
cancelAllQueries()call has been added before the wait loop, improving cleanup robustness as suggested in the previous review.
146-374: Comprehensive test coverage.The test methods provide thorough coverage of predicate pushdown scenarios across string matching, numeric comparisons, logical operators, and special functions. The test structure is clear and maintainable.
467-496: Helper methods are well-designed.
equalsIgnoreBasecorrectly normalizes memory addresses for stable comparisons, and the layout handle extraction helpers safely handle type checks. The logic is sound.
| long maxCleanUpTime = 5 * 1000L; // 5 seconds | ||
| long currentCleanUpTime = 0L; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract timeout constants for maintainability.
The timeout values and polling intervals are hardcoded at multiple locations (5 seconds, 60 seconds, 1-second intervals). Extracting these to named constants improves clarity and makes timeout adjustments easier.
Add constants at the class level:
@Test(singleThreaded = true)
public class TestClpComputePushDown
{
private static final String TABLE_NAME = "test_pushdown";
private static final Pattern BASE_PTR =
Pattern.compile("(?<=base=)\\[B@[0-9a-fA-F]+");
+ private static final long QUERY_CLEANUP_TIMEOUT_MS = 5_000L;
+ private static final long QUERY_PLANNING_TIMEOUT_MS = 60_000L;
+ private static final long POLL_INTERVAL_MS = 1_000L;Then reference these constants at lines 134-135 and 457-461.
Also applies to: 457-461
🤖 Prompt for AI Agents
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java
around lines 134-135 and 457-461: extract the hardcoded timeout and polling
values into descriptive class-level constants (e.g., CLEANUP_TIMEOUT_MS =
5_000L, GLOBAL_TIMEOUT_MS = 60_000L, POLL_INTERVAL_MS = 1_000L) and replace the
inline literals at the referenced lines with these constants; ensure constants
are static final and used everywhere the 5s, 60s, and 1s values appear in this
test class so future adjustments are centralized.
| 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; | ||
| } |
There was a problem hiding this comment.
Address potential query leak if planning fails.
If createQuery().get() throws an exception at line 456, or the assertion fails at line 463, the QueryId is created but not returned. The caller's finally block cannot cancel it. While tearDown() eventually calls cancelAllQueries(), the query remains active until then, potentially interfering with subsequent tests.
Apply this diff to ensure cleanup:
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;
+ try {
+ 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;
+ }
+ catch (Exception e) {
+ queryManager.cancelQuery(id);
+ throw e;
}
- assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime);
- return id;
}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (2)
436-438: Improve exception handling to preserve context.The current exception handling at lines 436-438 loses the stack trace. While the
finallyblock ensures query cleanup, propagating the full exception with context would aid debugging.Consider this approach:
- catch (Exception e) { - fail(e.getMessage()); - } + catch (Exception e) { + fail(format("Test failed for filter '%s': %s", filter, e.getMessage()), e); + }
449-468: Add try-catch to prevent query leaks during planning failures.If
createQuery().get()throws an exception or the assertion fails, the query has been created but may not be properly cancelled immediately. While the caller'sfinallyblock will eventually cancel it, adding a try-catch here ensures immediate cleanup.Apply this diff:
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); + try { 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; + } + catch (Exception e) { + queryManager.cancelQuery(id); + throw e; + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java(1 hunks)
🔇 Additional comments (4)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java (4)
58-64: LGTM: Well-structured test class with proper isolation.The
@Test(singleThreaded = true)annotation ensures test isolation, and theBASE_PTRpattern provides a clever way to normalize object references in assertions.
71-127: LGTM: Comprehensive test setup.The setup method properly initializes the mock metadata database with diverse column types, configures split filtering, and establishes the necessary query infrastructure for end-to-end pushdown testing.
146-374: LGTM: Comprehensive test coverage.The test methods systematically cover all major pushdown scenarios including string matching, numeric comparisons, boolean logic, special predicates (BETWEEN, IN, IS NULL), and CLP-specific functionality. The test organization by predicate type enhances readability.
481-499: LGTM: Clean helper methods for plan inspection.The helper methods for extracting
ClpTableLayoutHandlefrom plan nodes are straightforward, type-safe, and correctly handle the case where the expected node types are not found.
| @AfterClass | ||
| public void tearDown() | ||
| throws InterruptedException | ||
| { | ||
| queryRunner.cancelAllQueries(); | ||
| 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(); | ||
| } | ||
| deleteConfigFile(splitFilterConfigFilePath); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Teardown is adequate, but consider extracting timeout constants.
The teardown properly cancels all queries and waits for cleanup. However, the hardcoded timeout values (5 seconds, 1-second polling) would benefit from being extracted to class-level constants for maintainability.
Also applies to: 460-464
🤖 Prompt for AI Agents
In
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java
around lines 129-144 (and similarly at lines 460-464), the teardown method uses
hardcoded timeout/polling values (5 * 1000L and 1000L); extract these magic
numbers into descriptive private static final class-level constants (e.g.,
CLEANUP_MAX_WAIT_MS and CLEANUP_POLL_INTERVAL_MS), replace the literals in the
loop with those constants, and update any other occurrences at lines 460-464 to
use the same constants for consistency and maintainability.
| Thread.sleep(1000L); | ||
| currentWaitingTime += 1000L; | ||
| } | ||
| assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a more descriptive timeout assertion message.
The assertion provides a generic failure message. A descriptive message would help diagnose whether query planning is legitimately slow or the timeout needs adjustment.
- assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime);
+ assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime,
+ format("Query planning timed out after %d ms (filter: %s)", currentWaitingTime, filter));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); | |
| 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, | |
| format("Query planning timed out after %d ms (filter: %s)", currentWaitingTime, filter)); | |
| return id; | |
| } |
🤖 Prompt for AI Agents
In
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java
around line 466, the assertion assertTrue(currentWaitingTime <
maxDispatchingAndPlanningTime) lacks a descriptive failure message; replace it
with an assertion that includes a clear message and the actual timing values
(for example use assertTrue(condition, "Query planning exceeded timeout: waited
" + currentWaitingTime + " ms, allowed " + maxDispatchingAndPlanningTime + "
ms") or an equivalent assertion helper) so test failures show whether planning
was slow or the timeout needs adjustment.
| 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); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM: Clever normalization approach.
The equalsIgnoreBase method correctly normalizes base pointer references in RowExpression strings for consistent assertions. Consider adding a brief comment explaining why base pointer normalization is necessary for test reliability.
🤖 Prompt for AI Agents
In
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpComputePushDown.java
around lines 470 to 479, add a short inline comment above the equalsIgnoreBase
method explaining that base pointer normalization is necessary because
RowExpression.toString includes non-deterministic object base pointers (e.g.,
memory addresses) which would make string comparisons flaky in tests, so the
method replaces those patterns with a constant token to ensure stable
assertions; keep the comment concise (one or two sentences) and place it
directly above the method signature.
1 participant
Description
This PR can only be merged after #77 gets merged.
This PR refactored the split filter pushdown unit tests to use
ClpQueryRunner.All unit tests in
TestClpFilterToKqlhave been moved toTestClpComputePushDown, so it can be deleted.Checklist
breaking change.
Validation performed
Passed the CI.
Summary by CodeRabbit
Tests
Chores