feat: Ensure the timestamp in the generated KQL pushdown always represents in nanoseconds. #76
Conversation
WalkthroughAdds nanosecond timestamp handling to CLP filter-to-KQL conversion and treats TIMESTAMP and TIMESTAMP_MICROSECONDS as numeric-compatible; updates BETWEEN and literal handling. Adds a push-down integration test for timestamp comparisons and a SessionContext test helper for tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Dispatch as DispatchManager
participant Planner
participant Scan as TableScanNode
participant CLP as CLP Layout/Metadata
participant Converter as ClpFilterToKqlConverter
Client->>Dispatch: submit SQL
Dispatch->>Planner: request plan
Planner->>Scan: create TableScanNode
Scan->>CLP: request layout with filters
CLP->>Converter: provide filter expressions
Note right of Converter #D3E4CD: Convert TIMESTAMP / TIMESTAMP_MICROSECONDS\nto nanoseconds for literals\nApply conversion in BETWEEN and literal handling
Converter-->>CLP: KQL filter string (nanosecond timestamps)
CLP-->>Scan: layout including push-down
Planner-->>Dispatch: return plan with push-down
Dispatch-->>Client: execute / run query
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)
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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java(5 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java(1 hunks)presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
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/TestClpPushDown.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-clp/src/test/java/com/facebook/presto/plugin/clp/mockdb/table/ColumnMetadataTableRows.java (1)
ColumnMetadataTableRows(30-63)presto-clp/src/test/java/com/facebook/presto/plugin/clp/ClpQueryRunner.java (1)
ClpQueryRunner(28-101)
⏰ 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). (5)
- GitHub Check: test (17.0.13, :presto-tests -P ci-only-distributed-non-hash-gen)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-general)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestissimo-worker-images-build
🔇 Additional comments (10)
presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java (4)
19-19: LGTM!The new imports for
TimestampType, timestamp constants, andSECONDStime unit are necessary for the nanosecond timestamp conversion logic.Also applies to: 61-62, 69-69
263-266: LGTM!The extraction of bound types and conversion to nanoseconds ensures that BETWEEN operators work correctly with timestamp literals. The conversion handles both TIMESTAMP and TIMESTAMP_MICROSECONDS types appropriately.
449-449: LGTM!Converting the literal to nanoseconds before building the KQL expression ensures consistency with CLP's nanosecond timestamp storage.
924-926: LGTM!Treating
TIMESTAMPandTIMESTAMP_MICROSECONDSas CLP-compatible numeric types is correct, as these types can be compared using numeric operators in the generated KQL.presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestingClpSessionContext.java (3)
40-43: LGTM!The constructor correctly validates that the session is non-null, preventing potential NullPointerExceptions during test execution.
52-61: LGTM!The defaults for catalog ("clp") and schema ("default") are appropriate for CLP test contexts and align with the test setup in
TestClpPushDownandClpQueryRunner.
129-137: LGTM!The transformation from
ConnectorId-keyed connector properties to catalog-name-keyed properties correctly adapts theSessionAPI to theSessionContextAPI requirements. The use ofImmutableMap.Builderensures thread-safety for the test context.presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java (3)
53-54: LGTM!The test constants are well-defined, and the nanosecond value (1746003005000000000) is correctly calculated as
TEST_TS_SECONDS * 1_000_000_000.
62-98: LGTM!The test setup correctly:
- Creates a mock metadata database with H2
- Registers the test table with appropriate columns including a timestamp column
- Configures the CLP metadata provider
- Creates a distributed query runner with zero workers for testing
This aligns with the integration testing approach described in the PR objectives.
113-129: LGTM!The test comprehensively validates timestamp comparison operators:
- All standard comparison operators (>, >=, <, <=, =, !=, <>)
- Both orderings (column-to-literal and literal-to-column)
- Proper KQL generation for each operator type
This ensures the nanosecond conversion works correctly across all supported comparison patterns.
| 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); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the timestamp conversion logic handles edge cases correctly.
The conversion logic looks correct for standard cases:
- Extracts epoch seconds and nanosecond fraction from the timestamp representation
- Converts to total nanoseconds using
SECONDS.toNanos(seconds) + nanosecondFraction
However, ensure this handles edge cases:
- Negative timestamps (dates before 1970-01-01)
- Very large timestamps (dates far in the future)
- Overflow when converting seconds to nanoseconds
🏁 Script executed:
#!/bin/bash
# Description: Search for TimestampType method usage to understand the conversion behavior
# Check how getEpochSecond and getNanos are used elsewhere in the codebase
rg -nP --type=java -A3 -B3 'getEpochSecond|getNanos' -g '!**/test/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
# Locate TimestampType.java and inspect getEpochSecond and getNanos implementations
file=$(fd -t f TimestampType.java | head -n1)
rg -nC3 'getEpochSecond|getNanos' "$file"Length of output: 485
Prevent overflow in ensureNanosecondTimestamp
In ensureNanosecondTimestamp (ClpFilterToKqlConverter.java:929-947), converting seconds to nanoseconds via SECONDS.toNanos(seconds) can overflow a 64-bit long for TIMESTAMP_MICROSECONDS; add overflow checks or switch to BigInteger.
🤖 Prompt for AI Agents
In
presto-clp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java
around lines 929-947, the current SECONDS.toNanos(seconds) can overflow a 64-bit
long for TIMESTAMP_MICROSECONDS; change the computation to avoid overflow by
using BigInteger for the multiplication/addition (or at minimum check for
overflow before multiplying). Specifically, compute nanoseconds as
BigInteger.valueOf(seconds).multiply(BigInteger.valueOf(1_000_000_000)).add(BigInteger.valueOf(nanosecondFraction)).toString()
and return that string (or fall back to BigInteger only when the multiplication
would overflow), preserving existing parsing and return behavior.
| 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); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider reducing the maximum wait time or making it configurable.
A 60-second maximum wait time for the query to reach RUNNING state seems excessive for unit tests and may slow down the test suite. Consider:
- Reducing the wait time to 10-15 seconds for faster feedback
- Making the timeout configurable via system property for different environments
- Adding more informative failure messages if the timeout is exceeded
Apply this diff to reduce the wait time and improve the error message:
- long maxDispatchingAndPlanningTime = 60 * 1000L; // 1 minute
+ long maxDispatchingAndPlanningTime = 15 * 1000L; // 15 seconds
long currentWaitingTime = 0L;
while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() && currentWaitingTime < maxDispatchingAndPlanningTime) {
Thread.sleep(1000L);
currentWaitingTime += 1000L;
}
- assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime);
+ assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime,
+ format("Query did not reach RUNNING state within %dms. Current state: %s",
+ maxDispatchingAndPlanningTime, dispatchManager.getQueryInfo(id).getState()));📝 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.
| 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); | |
| long maxDispatchingAndPlanningTime = 15 * 1000L; // 15 seconds | |
| long currentWaitingTime = 0L; | |
| while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() | |
| && currentWaitingTime < maxDispatchingAndPlanningTime) { | |
| Thread.sleep(1000L); | |
| currentWaitingTime += 1000L; | |
| } | |
| assertTrue( | |
| currentWaitingTime < maxDispatchingAndPlanningTime, | |
| format( | |
| "Query did not reach RUNNING state within %dms. Current state: %s", | |
| maxDispatchingAndPlanningTime, | |
| dispatchManager.getQueryInfo(id).getState() | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java
around lines 141-147, replace the hard-coded 60_000L timeout with a configurable
timeout (read a System property like "test.dispatch.timeoutMillis" and default
to 15_000L), use that value in the loop and waiting-time accumulation, and
update the final assertion to include a clear failure message showing the query
id, final state and elapsed wait time so failures are informative; ensure
parsing of the system property is safe with a fallback to the default.
| boolean isPushDownGenerated = false; | ||
| for (Map.Entry<PlanNodeId, PlanNode> 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); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve error handling for missing TableScanNode.
If no TableScanNode with a ClpTableLayoutHandle is found, the test fails with "isPushDownGenerated is false", which doesn't provide much context. Consider adding more informative error messages to help diagnose test failures.
Apply this diff to improve error reporting:
boolean isPushDownGenerated = false;
+ StringBuilder debugInfo = new StringBuilder("Available plan nodes:\n");
for (Map.Entry<PlanNodeId, PlanNode> entry : queryManager.getFullQueryInfo(id).getPlanIdNodeMap().entrySet()) {
+ debugInfo.append(format(" %s: %s%n", entry.getKey(), entry.getValue().getClass().getSimpleName()));
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);
+ assertTrue(isPushDownGenerated,
+ format("No TableScanNode with ClpTableLayoutHandle found in query plan.%n%s", debugInfo));
queryManager.cancelQuery(id);📝 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.
| boolean isPushDownGenerated = false; | |
| for (Map.Entry<PlanNodeId, PlanNode> 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); | |
| boolean isPushDownGenerated = false; | |
| StringBuilder debugInfo = new StringBuilder("Available plan nodes:\n"); | |
| for (Map.Entry<PlanNodeId, PlanNode> entry : queryManager.getFullQueryInfo(id).getPlanIdNodeMap().entrySet()) { | |
| debugInfo.append(format(" %s: %s%n", entry.getKey(), entry.getValue().getClass().getSimpleName())); | |
| 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, | |
| format("No TableScanNode with ClpTableLayoutHandle found in query plan.%n%s", debugInfo)); | |
| queryManager.cancelQuery(id); |
🤖 Prompt for AI Agents
In presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java
around lines 148 to 160, the test currently fails with an unhelpful
"isPushDownGenerated is false" when no TableScanNode with a ClpTableLayoutHandle
is found and uses assertEquals without context; update the test to throw/assert
with informative messages: when iterating, collect or detect absence and after
loop call fail(...) or assertTrue(..., "No TableScanNode with
ClpTableLayoutHandle found for query id=" + id) to include the query id and any
identifying info; also change the assertEquals to include an assertion message
showing expected and actual pushdown (e.g. assertEquals("Pushdown mismatch for
query id=" + id + ": expected=<" + expectedPushDown + "> actual=<" +
actualPushDown + ">", expectedPushDown, actualPushDown)) so failures give clear
diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java (2)
155-161: Shorten/configure dispatch wait and emit informative failures.The 60 s fixed wait still causes slow, opaque failures—see the earlier review note. Please read a system property (e.g.,
test.dispatch.timeoutMillis, default 15 000 ms), reuse it in the loop, and include the query id, final state, and elapsed time in the assertion message so flakes are diagnosable.- long maxDispatchingAndPlanningTime = 60 * 1000L; // 1 minute + long maxDispatchingAndPlanningTime = Long.getLong("test.dispatch.timeoutMillis", 15_000L); long currentWaitingTime = 0L; while (dispatchManager.getQueryInfo(id).getState().ordinal() != RUNNING.ordinal() && currentWaitingTime < maxDispatchingAndPlanningTime) { Thread.sleep(1000L); currentWaitingTime += 1000L; } - assertTrue(currentWaitingTime < maxDispatchingAndPlanningTime); + assertTrue( + currentWaitingTime < maxDispatchingAndPlanningTime, + format("Query %s stuck in state %s after %d ms (limit %d ms)", + id, + dispatchManager.getQueryInfo(id).getState(), + currentWaitingTime, + maxDispatchingAndPlanningTime));
162-174: Capture plan context when pushdown is missing or mismatched.When the test fails today we only see “isPushDownGenerated is false” with no clue which nodes were present or what KQL string was produced—exactly the diagnostic gap flagged previously. Buffer the encountered nodes, surface the query id, and attach the expected/actual strings in the assertion message.
- boolean isPushDownGenerated = false; + boolean isPushDownGenerated = false; + StringBuilder planDebugInfo = new StringBuilder("Plan nodes:\n"); for (Map.Entry<PlanNodeId, PlanNode> entry : queryManager.getFullQueryInfo(id).getPlanIdNodeMap().entrySet()) { + planDebugInfo.append(format(" %s -> %s%n", entry.getKey(), entry.getValue().getClass().getSimpleName())); 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); + assertEquals( + actualPushDown, + expectedPushDown, + format("Pushdown mismatch for query %s.%nExpected: %s%nActual: %s%n%s", id, expectedPushDown, actualPushDown, planDebugInfo)); isPushDownGenerated = true; break; } - assertTrue(isPushDownGenerated); + assertTrue( + isPushDownGenerated, + format("No TableScanNode with ClpTableLayoutHandle found for query %s.%n%s", id, planDebugInfo));
📜 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/TestClpPushDown.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java (3)
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/ClpQueryRunner.java (1)
ClpQueryRunner(28-101)
⏰ 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). (9)
- GitHub Check: test (17.0.13, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: test (8.0.442, :presto-tests -P ci-only-distributed-queries)
- GitHub Check: test (8.0.442, :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-aggregation-queries)
- GitHub Check: test (8.0.442, :presto-tests -P presto-tests-execution-memory)
- GitHub Check: maven-checks (8.0.442)
- GitHub Check: maven-checks (17.0.13)
- GitHub Check: prestissimo-worker-images-build
| @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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure queryRunner is closed during teardown.
queryRunner spins up threads and in-memory services; leaving it open after each test leaks resources across the suite. Close it (and null it out) once outstanding queries are drained, handling exceptions so failures remain debuggable.
while (!queryManager.getQueries().isEmpty() && currentCleanUpTime < maxCleanUpTime) {
Thread.sleep(1000L);
currentCleanUpTime += 1000L;
}
- if (null != mockMetadataDatabase) {
+ try {
+ if (queryRunner != null) {
+ queryRunner.close();
+ }
+ }
+ catch (Exception e) {
+ fail("Failed to close queryRunner", e);
+ }
+ finally {
+ queryRunner = null;
+ }
+ if (mockMetadataDatabase != null) {
mockMetadataDatabase.teardown();
}📝 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.
| @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(); | |
| } | |
| } | |
| @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; | |
| } | |
| try { | |
| if (queryRunner != null) { | |
| queryRunner.close(); | |
| } | |
| } | |
| catch (Exception e) { | |
| fail("Failed to close queryRunner", e); | |
| } | |
| finally { | |
| queryRunner = null; | |
| } | |
| if (mockMetadataDatabase != null) { | |
| mockMetadataDatabase.teardown(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpPushDown.java
around lines 104 to 117, the tearDown method drains outstanding queries but does
not close the queryRunner, which leaks threads and in-memory services; after
waiting for queries to drain, call queryRunner.close() inside a try/catch so any
exception is logged or rethrown for debuggability, then set queryRunner = null
to avoid reuse; do this before or after mockMetadataDatabase.teardown() as
appropriate and ensure exceptions are handled (logged) so test failures remain
debuggable.
anlowee
force-pushed
the
xwei/ensure-ts-nanoseconds
branch
from
04be61a to
d2bec6f
Compare
1 participant
Description
This PR ensures the timestamp to always represents in nanoseconds to avoid unexpected comparison results when filter log events by timestamp. This PR should be merged after the CLP finishes the timestamp refactor, where all timestamps are stored in nanoseconds.
This PR also enhances the unit test framework. Previously we skipped other plan optmizers and directly used our own plan optimizers, which caused the expression like
from_unittime(1234)which is supposed to be a constant but turns out to be a call expression whenClpFilterToKqlConverterhandles it, thus it will be skipped due to that it is not considered as a constant. This PR uses theClpQueryRunnerwhich is already used in end-to-end testing to test the KQL generation, which makes more sense and can translate all call expressions which are actually constants before the execution entersClpFilterToKqlConverterso that it can test the cases for timestamp comparisons.Must be merged after this PR #77 gets merged.
Checklist
breaking change.
Validation performed
End-to-end testing and observe coordinator logs that the nanosecond pushdown is generated.
Passed the CI.
Summary by CodeRabbit
New Features
Tests
Chores