mirror of
https://github.com/hyperdxio/hyperdx
synced 2026-04-21 13:37:15 +00:00
fix: Ensure correct bounds for date-based timestampValueExpr (#2066)
## Summary This PR fixes the time filter expression generated when a source's timestamp value expression includes a Date-type column. In such cases, the bounds should be inclusive for the date-type columns. This extends the fix from #1915 to date-type columns. ### Screenshots or video #### Before - Histogram is empty because of exclusive end bounds <img width="1758" height="700" alt="Screenshot 2026-04-07 at 12 32 05 PM" src="https://github.com/user-attachments/assets/95898655-21f1-4380-9b23-5333fcea007f" /> #### After - Histogram is populated <img width="1762" height="717" alt="Screenshot 2026-04-07 at 12 30 25 PM" src="https://github.com/user-attachments/assets/d159b131-1f4f-44f8-982e-b26ca68835ff" /> ### How to test locally or on Vercel The unit tests demonstrate the fix. <details> <summary>Alternatively, create a table with the following schema and data</summary> ```sql CREATE TABLE default.otel_logs_date_pk ( `Timestamp` DateTime64(9) CODEC(Delta(8), ZSTD(1)), `TimestampTime` DateTime DEFAULT toDateTime(Timestamp), `TimestampDate` Date DEFAULT toDate(Timestamp), `TraceId` String CODEC(ZSTD(1)), `SpanId` String CODEC(ZSTD(1)), `TraceFlags` UInt8, `SeverityText` LowCardinality(String) CODEC(ZSTD(1)), `SeverityNumber` UInt8, `ServiceName` LowCardinality(String) CODEC(ZSTD(1)), `Body` String CODEC(ZSTD(1)), `ResourceSchemaUrl` LowCardinality(String) CODEC(ZSTD(1)), `ResourceAttributes` Map(LowCardinality(String), String) CODEC(ZSTD(1)), `ScopeSchemaUrl` LowCardinality(String) CODEC(ZSTD(1)), `ScopeName` String CODEC(ZSTD(1)), `ScopeVersion` LowCardinality(String) CODEC(ZSTD(1)), `ScopeAttributes` Map(LowCardinality(String), String) CODEC(ZSTD(1)), `LogAttributes` Map(LowCardinality(String), String) CODEC(ZSTD(1)), `__hdx_materialized_k8s.cluster.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.cluster.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.container.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.container.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.deployment.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.deployment.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.namespace.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.namespace.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.node.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.node.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.pod.name` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.pod.name'] CODEC(ZSTD(1)), `__hdx_materialized_k8s.pod.uid` LowCardinality(String) MATERIALIZED ResourceAttributes['k8s.pod.uid'] CODEC(ZSTD(1)), `__hdx_materialized_deployment.environment.name` LowCardinality(String) MATERIALIZED ResourceAttributes['deployment.environment.name'] CODEC(ZSTD(1)), INDEX idx_trace_id TraceId TYPE bloom_filter(0.001) GRANULARITY 1, INDEX idx_res_attr_key mapKeys(ResourceAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_res_attr_value mapValues(ResourceAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_scope_attr_key mapKeys(ScopeAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_scope_attr_value mapValues(ScopeAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_log_attr_key mapKeys(LogAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_log_attr_value mapValues(LogAttributes) TYPE bloom_filter(0.01) GRANULARITY 1, INDEX idx_lower_body lower(Body) TYPE tokenbf_v1(32768, 3, 0) GRANULARITY 8 ) ENGINE = MergeTree PARTITION BY toDate(TimestampTime) PRIMARY KEY (TimestampDate, ServiceName, TimestampTime) ORDER BY (TimestampDate, ServiceName, TimestampTime, Timestamp) TTL TimestampTime + toIntervalDay(1) SETTINGS index_granularity = 8192, ttl_only_drop_parts = 1; insert into default.otel_logs_date_pk (Timestamp, SeverityText, Body) VALUES (now(), 'info', 'message'); insert into default.otel_logs_date_pk (Timestamp, SeverityText, Body) VALUES (now()-interval 1 minute, 'info', 'message'); ``` Then create a source that points to that table with the timestampValueExpr `TimestampTime, TimestampDate` </details> ### References - Linear Issue: Closes HDX-3930 - Related PRs:
This commit is contained in:
Drew Davis
GitHub
parent
ad71dc2e91
commit
24767c5886
3 changed files with 99 additions and 7 deletions
5
.changeset/hot-camels-sing.md
Normal file
5
.changeset/hot-camels-sing.md
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
---
|
||||
"@hyperdx/common-utils": patch
|
||||
---
|
||||
|
||||
fix: Ensure correct bounds for date-based timestampValueExpr
|
||||
|
|
@ -1293,6 +1293,52 @@ describe('renderChartConfig', () => {
|
|||
dateRangeStartInclusive: false,
|
||||
expected: `(toStartOfHour(timestamp) >= toStartOfHour(fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()})) AND toStartOfHour(timestamp) <= toStartOfHour(fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()})))`,
|
||||
},
|
||||
{
|
||||
description: 'stays inclusive with date-type column',
|
||||
timestampValueExpression: 'date',
|
||||
dateRange: [
|
||||
new Date('2025-02-12 03:53:38Z'),
|
||||
new Date('2025-02-12 04:08:38Z'),
|
||||
],
|
||||
dateRangeStartInclusive: false,
|
||||
dateRangeEndInclusive: false,
|
||||
expected: `(date >= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()})) AND date <= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()})))`,
|
||||
},
|
||||
{
|
||||
description:
|
||||
'stays inclusive for date-type column in multi-column timestampValueExpression',
|
||||
timestampValueExpression: 'date, timestamp',
|
||||
dateRange: [
|
||||
new Date('2025-02-12 03:53:38Z'),
|
||||
new Date('2025-02-12 04:08:38Z'),
|
||||
],
|
||||
dateRangeStartInclusive: false,
|
||||
dateRangeEndInclusive: false,
|
||||
expected: `(date >= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()})) AND date <= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()})))AND(timestamp > fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()}) AND timestamp < fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()}))`,
|
||||
},
|
||||
{
|
||||
description: 'stays inclusive for toDate column',
|
||||
timestampValueExpression: 'toDate(timestamp)',
|
||||
dateRange: [
|
||||
new Date('2025-02-12 03:53:38Z'),
|
||||
new Date('2025-02-12 04:08:38Z'),
|
||||
],
|
||||
dateRangeStartInclusive: false,
|
||||
dateRangeEndInclusive: false,
|
||||
expected: `(toDate(timestamp) >= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()})) AND toDate(timestamp) <= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()})))`,
|
||||
},
|
||||
{
|
||||
description:
|
||||
'stays inclusive for toDate column in multi-column timestampValueExpression',
|
||||
timestampValueExpression: 'toDate(timestamp), timestamp',
|
||||
dateRange: [
|
||||
new Date('2025-02-12 03:53:38Z'),
|
||||
new Date('2025-02-12 04:08:38Z'),
|
||||
],
|
||||
dateRangeStartInclusive: false,
|
||||
dateRangeEndInclusive: false,
|
||||
expected: `(toDate(timestamp) >= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()})) AND toDate(timestamp) <= toDate(fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()})))AND(timestamp > fromUnixTimestamp64Milli(${new Date('2025-02-12 03:53:38Z').getTime()}) AND timestamp < fromUnixTimestamp64Milli(${new Date('2025-02-12 04:08:38Z').getTime()}))`,
|
||||
},
|
||||
];
|
||||
|
||||
beforeEach(() => {
|
||||
|
|
@ -1338,6 +1384,36 @@ describe('renderChartConfig', () => {
|
|||
expect(actualSql).toBe(expected);
|
||||
},
|
||||
);
|
||||
|
||||
it('stays inclusive for date-type column with non-subquery with clauses', async () => {
|
||||
const dateRange: [Date, Date] = [
|
||||
new Date('2025-02-12 03:53:38Z'),
|
||||
new Date('2025-02-12 04:08:38Z'),
|
||||
];
|
||||
|
||||
const actual = await timeFilterExpr({
|
||||
timestampValueExpression: 'date',
|
||||
dateRangeEndInclusive: false,
|
||||
dateRangeStartInclusive: false,
|
||||
dateRange,
|
||||
connectionId: 'test-connection',
|
||||
databaseName: 'default',
|
||||
tableName: 'target_table',
|
||||
metadata: mockMetadata,
|
||||
with: [
|
||||
{
|
||||
name: 'service',
|
||||
sql: { sql: 'ServiceName', params: {} },
|
||||
isSubquery: false,
|
||||
},
|
||||
],
|
||||
});
|
||||
|
||||
const actualSql = parameterizedQueryToSql(actual);
|
||||
expect(actualSql).toBe(
|
||||
`(date >= toDate(fromUnixTimestamp64Milli(${dateRange[0].getTime()})) AND date <= toDate(fromUnixTimestamp64Milli(${dateRange[1].getTime()})))`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it('should not generate invalid SQL when primary key wraps toStartOfInterval', async () => {
|
||||
|
|
|
|||
|
|
@ -686,8 +686,11 @@ export async function timeFilterExpr({
|
|||
// timestamp comparison must also have the same function
|
||||
const toStartOf = parseToStartOfFunction(col);
|
||||
|
||||
// Detect toDate(...) wrapper expressions
|
||||
const isToDateExpr = /^toDate\s*\(/.test(col);
|
||||
|
||||
const columnMeta =
|
||||
withClauses?.length || toStartOf
|
||||
hasSubqueryCte(withClauses) || toStartOf || isToDateExpr
|
||||
? null
|
||||
: await metadata.getColumn({
|
||||
databaseName,
|
||||
|
|
@ -700,7 +703,12 @@ export async function timeFilterExpr({
|
|||
UNSAFE_RAW_SQL: col,
|
||||
};
|
||||
|
||||
if (columnMeta == null && !withClauses?.length && !toStartOf) {
|
||||
if (
|
||||
columnMeta == null &&
|
||||
hasSubqueryCte(withClauses) &&
|
||||
!toStartOf &&
|
||||
!isToDateExpr
|
||||
) {
|
||||
console.warn(
|
||||
`Column ${col} not found in ${databaseName}.${tableName} while inferring type for time filter`,
|
||||
);
|
||||
|
|
@ -718,12 +726,15 @@ export async function timeFilterExpr({
|
|||
? chSql`${toStartOf.function}(fromUnixTimestamp64Milli(${{ Int64: endTime }})${toStartOf.formattedRemainingArgs})`
|
||||
: chSql`fromUnixTimestamp64Milli(${{ Int64: endTime }})`;
|
||||
|
||||
// toStartOf* filters must stay inclusive — strict < on a rounded value drops a whole interval
|
||||
const startOp = dateRangeStartInclusive || toStartOf ? '>=' : '>';
|
||||
const endOp = dateRangeEndInclusive || toStartOf ? '<=' : '<';
|
||||
const isDateType = columnMeta?.type === 'Date' || isToDateExpr;
|
||||
|
||||
// If it's a date type
|
||||
if (columnMeta?.type === 'Date') {
|
||||
// toStartOf* and Date filters must stay inclusive — strict < on a rounded value drops a whole interval
|
||||
const startOp =
|
||||
dateRangeStartInclusive || toStartOf || isDateType ? '>=' : '>';
|
||||
const endOp =
|
||||
dateRangeEndInclusive || toStartOf || isDateType ? '<=' : '<';
|
||||
|
||||
if (isDateType) {
|
||||
return chSql`(${unsafeTimestampValueExpression} ${startOp} toDate(${startTimeCond}) AND ${unsafeTimestampValueExpression} ${endOp} toDate(${endTimeCond}))`;
|
||||
} else {
|
||||
return chSql`(${unsafeTimestampValueExpression} ${startOp} ${startTimeCond} AND ${unsafeTimestampValueExpression} ${endOp} ${endTimeCond})`;
|
||||
|
|
|
|||
Loading…
Reference in a new issue