Skip to content

feat: Add timestamp() KQL filter pushdown; sync DeprecatedDateString rename with clp-s upstream. #148

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
20001020ycx merged 5 commits into from
Mar 3, 2026
Merged

feat: Add timestamp() KQL filter pushdown; sync DeprecatedDateString rename with clp-s upstream. #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4aab91f
feat(clp): Add Timestamp (byte 14) KQL filter pushdown and sync Depre…
20001020ycx Feb 25, 2026
8d6a12e
fix(clp): Update DateString reference to DeprecatedDateString in Test…
20001020ycx Feb 26, 2026
8e9b126
docs(clp): Add comment explaining the timezone-pinned SessionHolder c…
20001020ycx Feb 26, 2026
b17fd55
Merge branch 'release-0.297-edge-10-clp-connector' into feat/clp-time…
20001020ycx Mar 2, 2026
4e09a80
chore: update velox submodule to 739a3aac8f for Timestamp column type…
20001020ycx Mar 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ private Type mapColumnType(byte type)
case VarString:
case NullValue:
return VARCHAR;
case DateString:
case DeprecatedDateString:
case Timestamp:
return TimestampType.TIMESTAMP;
case UnstructuredArray:
return new ArrayType(VARCHAR);
Expand Down
5 changes: 3 additions & 2 deletions presto-clp/src/main/java/com/facebook/presto/plugin/clp/metadata/ClpSchemaTreeNodeType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@ public enum ClpSchemaTreeNodeType
Object((byte) 5),
UnstructuredArray((byte) 6),
NullValue((byte) 7),
DateString((byte) 8),
DeprecatedDateString((byte) 8),
StructuredArray((byte) 9),
FormattedFloat((byte) 12),
DictionaryFloat((byte) 13);
DictionaryFloat((byte) 13),
Timestamp((byte) 14);

private static final ClpSchemaTreeNodeType[] LOOKUP_TABLE;
private final byte type;
Expand Down
27 changes: 23 additions & 4 deletions ...lp/src/main/java/com/facebook/presto/plugin/clp/optimization/ClpFilterToKqlConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,9 @@ private ClpExpression handleBetween(CallExpression node)
String variable = variableOpt.get();
String lowerBound = getLiteralString((ConstantExpression) second);
String upperBound = getLiteralString((ConstantExpression) third);
String kql = String.format("%s >= %s AND %s <= %s", variable, lowerBound, variable, upperBound);
String kql = String.format("%s >= %s AND %s <= %s",
variable, formatLiteral(second.getType(), lowerBound),
variable, formatLiteral(third.getType(), upperBound));
String metadataSqlQuery = metadataFilterColumns.contains(variable)
? String.format("\"%s\" >= %s AND \"%s\" <= %s", variable, lowerBound, variable, upperBound)
: null;
Expand Down Expand Up @@ -450,7 +452,7 @@ private ClpExpression buildClpExpression(
if (metadataFilterColumns.contains(variableName)) {
metadataSqlQuery = format("\"%s\" = %s", variableName, literalString);
}
return new ClpExpression(format("%s: %s", variableName, literalString), metadataSqlQuery);
return new ClpExpression(format("%s: %s", variableName, formatLiteral(literalType, literalString)), metadataSqlQuery);
}
}
else if (operator.equals(NOT_EQUAL)) {
Expand All @@ -461,14 +463,14 @@ else if (operator.equals(NOT_EQUAL)) {
if (metadataFilterColumns.contains(variableName)) {
metadataSqlQuery = format("NOT \"%s\" = %s", variableName, literalString);
}
return new ClpExpression(format("NOT %s: %s", variableName, literalString), metadataSqlQuery);
return new ClpExpression(format("NOT %s: %s", variableName, formatLiteral(literalType, literalString)), metadataSqlQuery);
}
}
else if (LOGICAL_BINARY_OPS_FILTER.contains(operator) && !(literalType instanceof VarcharType)) {
if (metadataFilterColumns.contains(variableName)) {
metadataSqlQuery = format("\"%s\" %s %s", variableName, operator.getOperator(), literalString);
}
return new ClpExpression(format("%s %s %s", variableName, operator.getOperator(), literalString), metadataSqlQuery);
return new ClpExpression(format("%s %s %s", variableName, operator.getOperator(), formatLiteral(literalType, literalString)), metadataSqlQuery);
}
return new ClpExpression(originalNode);
}
Expand Down Expand Up @@ -888,6 +890,23 @@ private ClpExpression handleDereference(RowExpression expression)
return new ClpExpression(baseString.getPushDownExpression().get() + "." + fieldName);
}

/**
* Wraps a literal string in the KQL timestamp function for TIMESTAMP types.
* <p></p>
* Example: <code>"1672531200000"</code> → <code>timestamp("1672531200000", "\L")</code>
*
* @param literalType the type of the literal
* @param literalString the raw string representation of the literal
* @return the formatted literal string, wrapped if TIMESTAMP type
*/
private static String formatLiteral(Type literalType, String literalString)
{
if (literalType.equals(TIMESTAMP)) {
return format("timestamp(\"%s\", \"\\L\")", literalString);
}
return literalString;
}

/**
* See
* <a href="https://docs.yscope.com/clp/main/user-docs/reference-json-search-syntax">here
Expand Down
26 changes: 26 additions & 0 deletions presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpFilterToKql.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package com.facebook.presto.plugin.clp;

import com.facebook.presto.common.type.TimeZoneKey;
import com.facebook.presto.plugin.clp.optimization.ClpFilterToKqlConverter;
import com.facebook.presto.spi.ColumnHandle;
import com.facebook.presto.spi.relation.RowExpression;
Expand Down Expand Up @@ -271,6 +272,31 @@ public void testMetadataSqlGeneration()
testMetadataFilterColumns);
}

@Test
public void testTimestampPushDown()
{
// Use UTC to ensure stable epoch-ms values regardless of local timezone
SessionHolder sessionHolder = new SessionHolder(TimeZoneKey.UTC_KEY);

// Epoch ms for TIMESTAMP '2023-01-01 00:00:00.000' UTC = 1672531200000
testPushDown(sessionHolder, "clpTimestamp > TIMESTAMP '2023-01-01 00:00:00.000'",
"clpTimestamp > timestamp(\"1672531200000\", \"\\L\")", null);
testPushDown(sessionHolder, "clpTimestamp >= TIMESTAMP '2023-01-01 00:00:00.000'",
"clpTimestamp >= timestamp(\"1672531200000\", \"\\L\")", null);
testPushDown(sessionHolder, "clpTimestamp < TIMESTAMP '2023-01-01 00:00:00.000'",
"clpTimestamp < timestamp(\"1672531200000\", \"\\L\")", null);
testPushDown(sessionHolder, "clpTimestamp <= TIMESTAMP '2023-01-01 00:00:00.000'",
"clpTimestamp <= timestamp(\"1672531200000\", \"\\L\")", null);
testPushDown(sessionHolder, "clpTimestamp = TIMESTAMP '2023-01-01 00:00:00.000'",
"clpTimestamp: timestamp(\"1672531200000\", \"\\L\")", null);

// Epoch ms for TIMESTAMP '2023-01-02 00:00:00.000' UTC = 1672617600000
testPushDown(sessionHolder,
"clpTimestamp BETWEEN TIMESTAMP '2023-01-01 00:00:00.000' AND TIMESTAMP '2023-01-02 00:00:00.000'",
"clpTimestamp >= timestamp(\"1672531200000\", \"\\L\") AND clpTimestamp <= timestamp(\"1672617600000\", \"\\L\")",
null);
}

@Test
public void testClpWildcardUdf()
{
Expand Down
20 changes: 19 additions & 1 deletion presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpQueryBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import com.facebook.presto.SystemSessionProperties;
import com.facebook.presto.common.block.BlockEncodingManager;
import com.facebook.presto.common.type.RowType;
import com.facebook.presto.common.type.TimeZoneKey;
import com.facebook.presto.common.type.Type;
import com.facebook.presto.metadata.AnalyzePropertyManager;
import com.facebook.presto.metadata.CatalogManager;
Expand Down Expand Up @@ -52,6 +53,7 @@
import static com.facebook.presto.common.type.BigintType.BIGINT;
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
import static com.facebook.presto.common.type.DoubleType.DOUBLE;
import static com.facebook.presto.common.type.TimestampType.TIMESTAMP;
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
import static com.facebook.presto.metadata.FunctionAndTypeManager.createTestFunctionAndTypeManager;
import static com.facebook.presto.metadata.SessionPropertyManager.createTestingSessionPropertyManager;
Expand Down Expand Up @@ -91,8 +93,9 @@ public class TestClpQueryBase
RowType.field("Name", VARCHAR)))))));
protected static final ClpColumnHandle fare = new ClpColumnHandle("fare", DOUBLE);
protected static final ClpColumnHandle isHoliday = new ClpColumnHandle("isHoliday", BOOLEAN);
protected static final ClpColumnHandle clpTimestamp = new ClpColumnHandle("clpTimestamp", TIMESTAMP);
protected static final Map<VariableReferenceExpression, ColumnHandle> variableToColumnHandleMap =
Stream.of(city, fare, isHoliday)
Stream.of(city, fare, isHoliday, clpTimestamp)
.collect(toMap(
ch -> new VariableReferenceExpression(Optional.empty(), ch.getColumnName(), ch.getColumnType()),
ch -> ch));
Expand Down Expand Up @@ -133,6 +136,21 @@ public SessionHolder()
session = TestingSession.testSessionBuilder(createTestingSessionPropertyManager(new SystemSessionProperties().getSessionProperties())).build();
}

/**
* Creates a session with a fixed timezone. Use this when tests assert against hardcoded
* epoch-ms values derived from timestamp literals (e.g., {@code TIMESTAMP '2023-01-01
* 00:00:00.000'}), since Presto parses timestamp literals relative to the session timezone.
* Without pinning the timezone, the same literal produces different epoch-ms values on
* machines in different timezones, causing the assertions to fail.
*/
public SessionHolder(TimeZoneKey timeZoneKey)
{
connectorSession = SESSION;
session = TestingSession.testSessionBuilder(createTestingSessionPropertyManager(new SystemSessionProperties().getSessionProperties()))
.setTimeZoneKey(timeZoneKey)
.build();
Comment on lines +148 to +151
Copy link

@coderabbitai coderabbitai bot Feb 26, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, let's find and read the file to understand the context
fd -e java "TestClpQueryBase.java" -x cat -n {}

Repository: y-scope/presto

Length of output: 1694

🌐 Web query:

Presto Session toConnectorSession method API

💡 Result:

In PrestoDB (server-side, presto-main), com.facebook.presto.Session exposes two overloads that convert an engine Session into the SPI-facing ConnectorSession used by connectors:

  • public ConnectorSession toConnectorSession()
  • public ConnectorSession toConnectorSession(ConnectorId connectorId) (use this when you need connector-specific session properties for a particular connector/catalog) [1]

ConnectorSession is the session object passed into connector SPI entry points (for example, many methods on ConnectorMetadata take a ConnectorSession session parameter) [2].

Sources:
[1] Presto Session Javadoc (presto-main) https://javadoc.io/static/com.facebook.presto/presto-main/0.256/com/facebook/presto/Session.html
[2] Presto SPI ConnectorMetadata Javadoc (example of ConnectorSession usage) https://javadoc.io/static/com.facebook.presto/presto-spi/0.236/com/facebook/presto/spi/connector/ConnectorMetadata.html

🌐 Web query:

Presto ConnectorSession timezone properties

💡 Result:

In Presto connector code, the session time zone is not a generic getProperty("...") value. It’s a first-class attribute on ConnectorSession:

  • Read it with ConnectorSession#getTimeZoneKey() (returns a TimeZoneKey). [1]

How it gets set (what drives getTimeZoneKey()):

  • Presto client protocol: clients can send X-Presto-Time-Zone; if they don’t, Presto uses the coordinator/engine default time zone. [2]
  • JDBC driver: you can set the JDBC parameter timeZoneId (and historically via PrestoConnection.setTimeZoneId(String)). [3][4]

Related SQL-side visibility:

  • current_timezone() returns the current session time zone (IANA name or fixed offset). [5]

Sources:
[1] https://javadoc.io/static/com.facebook.presto/presto-main/0.285.1/com/facebook/presto/testing/TestingConnectorSession.html
[2] https://prestodb.github.io/docs/current/develop/client-protocol.html
[3] https://prestodb.io/docs/current/clients/java.html
[4] https://prestodb.github.io/docs/current/release/release-0.66.html
[5] https://prestodb.github.io/docs/current/functions/datetime.html

🌐 Web query:

Presto TestingConnectorSession.SESSION default timezone

💡 Result:

TestingConnectorSession.SESSION is just a static convenience ConnectorSession instance; the Javadoc does not document what time zone it uses by default (it only exposes getTimeZoneKey()). [1]

So the only reliable ways to know the “default timezone” are to inspect the actual TestingConnectorSession implementation for your Presto version or check it at runtime:

TimeZoneKey tz = TestingConnectorSession.SESSION.getTimeZoneKey();

More generally (outside this test helper), Presto’s session time zone typically comes from the client, and if not available, from the coordinator/server. [2]

Sources:
[1] Presto TestingConnectorSession Javadoc (shows SESSION and getTimeZoneKey()) https://javadoc.io/static/com.facebook.presto/presto-main/0.285.1/com/facebook/presto/testing/TestingConnectorSession.html
[2] Presto docs (session time zone behavior) https://prestodb.github.io/docs/current/release/release-0.66.html

🌐 Web query:

Presto Session setTimeZoneKey toConnectorSession propagation

💡 Result:

In Presto, Session.SessionBuilder#setTimeZoneKey(...) sets the timeZoneKey field that becomes part of the immutable Session object when you call build(). The Session then propagates that value to ConnectorSession via toConnectorSession(...) like this:

  1. Builder stores TZ: SessionBuilder#setTimeZoneKey(TimeZoneKey timeZoneKey) assigns this.timeZoneKey = timeZoneKey. [1]
  2. Session stores TZ: Session keeps it as private final TimeZoneKey timeZoneKey; and exposes getTimeZoneKey(). [1]
  3. Session bakes TZ into SqlFunctionProperties: Session#getSqlFunctionProperties() builds SqlFunctionProperties with .setTimeZoneKey(timeZoneKey). [1]
  4. toConnectorSession() wraps the Session: Session#toConnectorSession() returns new FullConnectorSession(this, ...). [1]
  5. FullConnectorSession captures the Session’s function properties: its constructor sets this.sqlFunctionProperties = session.getSqlFunctionProperties(); and returns them from getSqlFunctionProperties(). [2]

So the effective propagation path is:

SessionBuilder.setTimeZoneKey → Session.timeZoneKey → Session.getSqlFunctionProperties().timeZoneKey → FullConnectorSession.getSqlFunctionProperties().timeZoneKey → connector code reads it from ConnectorSession (typically via getSqlFunctionProperties()). [1][2]

Sources
[1] Presto Session.java (0.256) showing setTimeZoneKey, getSqlFunctionProperties().setTimeZoneKey(...), and toConnectorSession() creating FullConnectorSession. (raw.githubusercontent.com)
[2] Presto FullConnectorSession.java (0.256) showing it stores session.getSqlFunctionProperties() and exposes it via getSqlFunctionProperties(). (raw.githubusercontent.com)

Citations:

Align connectorSession timezone with session timezone in SessionHolder constructors.

The connectorSession is set to the static TestingConnectorSession.SESSION (which has a default timezone), while session is built with an explicit setTimeZoneKey(). When callers invoke getConnectorSession(), they receive a connector session with a misaligned timezone, causing non-deterministic behavior in timezone-sensitive assertions.

Replace connectorSession = SESSION; with connectorSession = session.toConnectorSession(); in both constructors (lines 133–135 and 146–149). The Session.toConnectorSession() method properly propagates the timezone through SqlFunctionProperties to the returned FullConnectorSession.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@presto-clp/src/test/java/com/facebook/presto/plugin/clp/TestClpQueryBase.java`
around lines 146 - 149, The SessionHolder constructors set connectorSession =
SESSION which uses a default timezone; change both constructors to set
connectorSession = session.toConnectorSession() so the connectorSession's
timezone matches the built session (use Session.toConnectorSession() to
propagate timezone via SqlFunctionProperties); update any references in the
SessionHolder class (constructors and getConnectorSession()) accordingly.

}

public ConnectorSession getConnectorSession()
{
return connectorSession;
Expand Down
4 changes: 2 additions & 2 deletions ...ion/src/test/java/com/facebook/presto/nativeworker/TestPrestoNativeClpGeneralQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import static com.facebook.presto.plugin.clp.ClpQueryRunner.DEFAULT_NUM_OF_WORKERS;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Boolean;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.ClpString;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.DateString;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.DeprecatedDateString;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.Integer;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.NullValue;
import static com.facebook.presto.plugin.clp.metadata.ClpSchemaTreeNodeType.UnstructuredArray;
Expand Down Expand Up @@ -88,7 +88,7 @@ protected void createTables()
UnstructuredArray,
UnstructuredArray,
NullValue,
DateString))));
DeprecatedDateString))));
mockMetadataDatabase.addSplits(ImmutableMap.of(DEFAULT_TABLE_NAME, new ArchivesTableRows(
ImmutableList.of("mongodb-processed-single-file-archive"),
ImmutableList.of(1679441694576L),
Expand Down
2 changes: 1 addition & 1 deletion presto-native-execution/velox
Open in desktop
Submodule velox updated 7 files
+1 −1 CMake/resolve_dependency_modules/clp.cmake
+16 −0 velox/connectors/clp/search_lib/ClpTimestampsUtils.h
+11 −0 velox/connectors/clp/search_lib/archive/ClpArchiveCursor.cpp
+26 −2 velox/connectors/clp/search_lib/archive/ClpArchiveVectorLoader.cpp
+6 −0 velox/connectors/clp/search_lib/ir/ClpIrCursor.cpp
+64 −7 velox/connectors/clp/tests/ClpConnectorTest.cpp
+ − velox/connectors/clp/tests/examples/test_5.v0.5.0.clps
Loading