Skip to content

Commit

Permalink
fix(sql): bug in SAMPLE BY queries after a query cache hit (#4990)
Browse files Browse the repository at this point in the history
  • Loading branch information
@jerrinot
jerrinot authored Oct 8, 2024
1 parent 5b50a4f commit 318c2be
Show file tree
Hide file tree
Showing 6 changed files with 214 additions and 50 deletions.
3 changes: 3 additions & 0 deletions .idea/codeStyles/Project.xml
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 10 additions & 1 deletion core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1784,12 +1784,21 @@ private RecordCursorFactory generateFill(QueryModel model, RecordCursorFactory g

int timestampIndex = groupByFactory.getMetadata().getColumnIndexQuiet(alias);

int samplingIntervalEnd = TimestampSamplerFactory.findIntervalEndIndex(fillStride.token, fillStride.position);
long samplingInterval = TimestampSamplerFactory.parseInterval(fillStride.token, samplingIntervalEnd, fillStride.position);
assert samplingInterval > 0;
assert samplingIntervalEnd < fillStride.token.length();
char samplingIntervalUnit = fillStride.token.charAt(samplingIntervalEnd);
TimestampSampler timestampSampler = TimestampSamplerFactory.getInstance(samplingInterval, samplingIntervalUnit, fillStride.position);

return new FillRangeRecordCursorFactory(
groupByFactory.getMetadata(),
groupByFactory,
fillFromFunc,
fillToFunc,
fillStride.token,
samplingInterval,
samplingIntervalUnit,
timestampSampler,
fillValues,
timestampIndex
);
Expand Down
64 changes: 43 additions & 21 deletions core/src/main/java/io/questdb/griffin/engine/groupby/FillRangeRecordCursorFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,13 @@
package io.questdb.griffin.engine.groupby;

import io.questdb.cairo.AbstractRecordCursorFactory;
import io.questdb.cairo.sql.Function;
import io.questdb.cairo.sql.NoRandomAccessRecordCursor;
import io.questdb.cairo.sql.Record;
import io.questdb.cairo.sql.*;
import io.questdb.cairo.sql.RecordCursor;
import io.questdb.cairo.sql.RecordCursorFactory;
import io.questdb.cairo.sql.RecordMetadata;
import io.questdb.cairo.sql.SymbolTable;
import io.questdb.griffin.PlanSink;
import io.questdb.griffin.SqlException;
import io.questdb.griffin.SqlExecutionContext;
Expand All @@ -36,7 +41,11 @@
import io.questdb.griffin.engine.functions.constants.TimestampConstant;
import io.questdb.log.Log;
import io.questdb.log.LogFactory;
import io.questdb.std.*;
import io.questdb.std.BinarySequence;
import io.questdb.std.BitSet;
import io.questdb.std.Long256;
import io.questdb.std.Misc;
import io.questdb.std.ObjList;
import io.questdb.std.str.CharSink;
import io.questdb.std.str.Utf8Sequence;
import org.jetbrains.annotations.NotNull;
Expand All @@ -53,10 +62,11 @@
public class FillRangeRecordCursorFactory extends AbstractRecordCursorFactory {
public static final Log LOG = LogFactory.getLog(FillRangeRecordCursorFactory.class);
private final RecordCursorFactory base;
private final FillRangeRecordCursor cursor = new FillRangeRecordCursor();
private final FillRangeRecordCursor cursor;
private final Function fromFunc;
private final RecordMetadata metadata;
private final CharSequence stride;
private final long samplingInterval;
private final char samplingIntervalUnit;
private final int timestampIndex;
private final Function toFunc;
private final ObjList<Function> valueFuncs;
Expand All @@ -66,18 +76,24 @@ public FillRangeRecordCursorFactory(
RecordCursorFactory base,
Function fromFunc,
Function toFunc,
CharSequence stride,
long samplingInterval,
char samplingIntervalUnit,
TimestampSampler timestampSampler,
ObjList<Function> fillValues,
int timestampIndex
) {
super(metadata);
this.base = base;
this.fromFunc = fromFunc;
this.toFunc = toFunc;
this.stride = stride;

// needed for the EXPLAIN plan
this.samplingInterval = samplingInterval;
this.samplingIntervalUnit = samplingIntervalUnit;
this.timestampIndex = timestampIndex;
this.valueFuncs = fillValues;
this.metadata = metadata;
this.cursor = new FillRangeRecordCursor(timestampSampler, fromFunc, toFunc, fillValues, timestampIndex);
}

@Override
Expand All @@ -101,7 +117,7 @@ public RecordCursor getCursor(SqlExecutionContext executionContext) throws SqlEx

final RecordCursor baseCursor = base.getCursor(executionContext);
try {
cursor.of(baseCursor, fromFunc, toFunc, stride, valueFuncs, timestampIndex, executionContext);
cursor.of(baseCursor, executionContext);
return cursor;
} catch (Throwable th) {
cursor.close();
Expand All @@ -125,7 +141,7 @@ public void toPlan(PlanSink sink) {
if (fromFunc != TimestampConstant.NULL || toFunc != TimestampConstant.NULL) {
sink.attr("range").val('(').val(fromFunc).val(',').val(toFunc).val(')');
}
sink.attr("stride").val('\'').val(stride).val('\'');
sink.attr("stride").val('\'').val(samplingInterval).val(samplingIntervalUnit).val('\'');

// print values omitting the timestamp column
// since we added an extra artificial null
Expand Down Expand Up @@ -174,6 +190,11 @@ private static class FillRangeRecordCursor implements NoRandomAccessRecordCursor

private final FillRangeRecord fillingRecord = new FillRangeRecord();
private final FillRangeTimestampConstant fillingTimestampFunc = new FillRangeTimestampConstant();
private final Function fromFunc;
private final int timestampIndex;
private final TimestampSampler timestampSampler;
private final Function toFunc;
private final ObjList<Function> valueFuncs;
private RecordCursor baseCursor;
private Record baseRecord;
private int bucketIndex;
Expand All @@ -185,10 +206,19 @@ private static class FillRangeRecordCursor implements NoRandomAccessRecordCursor
private long nextBucketTimestamp;
private BitSet presentRecords;
private int rangeBound;
private int timestampIndex;
private TimestampSampler timestampSampler;
private long toTimestamp;
private ObjList<Function> valueFuncs;

private FillRangeRecordCursor(TimestampSampler timestampSampler,
@NotNull Function fromFunc,
@NotNull Function toFunc,
ObjList<Function> valueFuncs,
int timestampIndex) {
this.timestampSampler = timestampSampler;
this.fromFunc = fromFunc;
this.toFunc = toFunc;
this.valueFuncs = valueFuncs;
this.timestampIndex = timestampIndex;
}

@Override
public void close() {
Expand Down Expand Up @@ -301,7 +331,7 @@ private void initBounds(Function fromFunc, Function toFunc) {
rangeBound = RANGE_UNBOUNDED;
}

private void initTimestamps(Function fromFunc, Function toFunc, CharSequence stride) throws SqlException {
private void initTimestamps(Function fromFunc, Function toFunc) {
if (fromFunc != TimestampConstant.NULL) {
fromTimestamp = fromFunc.getTimestamp(null);
}
Expand All @@ -310,7 +340,6 @@ private void initTimestamps(Function fromFunc, Function toFunc, CharSequence str
toTimestamp = toFunc.getTimestamp(null);
}

timestampSampler = TimestampSamplerFactory.getInstance(stride, 0);
timestampSampler.setStart(fromTimestamp);

nextBucketTimestamp = fromTimestamp;
Expand Down Expand Up @@ -353,20 +382,13 @@ private boolean notAtEndOfBitset() {

private void of(
RecordCursor baseCursor,
@NotNull Function fromFunc,
@NotNull Function toFunc,
CharSequence stride,
ObjList<Function> valueFuncs,
int timestampIndex,
SqlExecutionContext executionContext
) throws SqlException {
this.baseCursor = baseCursor;
this.timestampIndex = timestampIndex;
this.valueFuncs = valueFuncs;
Function.initNcFunctions(valueFuncs, baseCursor, executionContext);
fromFunc.init(baseCursor, executionContext);
toFunc.init(baseCursor, executionContext);
initTimestamps(fromFunc, toFunc, stride);
initTimestamps(fromFunc, toFunc);
if (presentRecords == null) {
presentRecords = new BitSet(toFunc != TimestampConstant.NULL ? timestampSampler.bucketIndex(toTimestamp) : DEFAULT_BITSET_SIZE);
}
Expand Down
84 changes: 58 additions & 26 deletions core/src/main/java/io/questdb/griffin/engine/groupby/TimestampSamplerFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,25 @@
public final class TimestampSamplerFactory {

/**
* Parses strings such as '10m', '3M', '5d', '12h', 'y', '35s'
* Find the end of the interval token in the input string. The interval token is expected to be a number followed by
* a single letter qualifier.
*
* @param cs the key
* @param cs input string
* @param position position in SQL text to report error against
* @return instance of appropriate TimestampSampler
* @throws SqlException when input string is invalid
* @return index of the first character after the interval token
* @throws SqlException when input string is not a valid interval token
*/
public static TimestampSampler getInstance(CharSequence cs, int position) throws SqlException {
public static int findIntervalEndIndex(CharSequence cs, int position) throws SqlException {
int k = -1;

if (cs == null) {
throw SqlException.$(position, "missing interval");
}

final int len = cs.length();
if (len == 0) {
throw SqlException.$(position, "expected interval qualifier");
}

// look for end of digits
for (int i = 0; i < len; i++) {
Expand All @@ -58,6 +62,10 @@ public static TimestampSampler getInstance(CharSequence cs, int position) throws
}
}

if (k == 0 && cs.charAt(0) == '-') {
throw SqlException.$(position, "negative interval is not allowed");
}

if (k == -1) {
throw SqlException.$(position + len, "expected interval qualifier");
}
Expand All @@ -67,37 +75,19 @@ public static TimestampSampler getInstance(CharSequence cs, int position) throws
throw SqlException.$(position + k, "expected single letter qualifier");
}

try {
final int n;
if (k == 0) {
n = 1;
} else {
n = Numbers.parseInt(cs, 0, k);
if (n == 0) {
throw SqlException.$(position, "zero is not a valid sample value");
}
}

return createTimestampSampler(n, cs.charAt(k), position + k);
} catch (NumericException ignore) {
// we are parsing a pre-validated number
// but we have to deal with checked exception anyway
assert false;
}

throw SqlException.$(position + k, "unsupported interval qualifier");
return k;
}

public static TimestampSampler getInstance(long period, CharSequence units, int position) throws SqlException {
if (units.length() == 1) {
return createTimestampSampler(period, units.charAt(0), position);
return getInstance(period, units.charAt(0), position);
}
// Just in case SqlParser will allow this in the future
throw SqlException.$(position, "expected one character interval qualifier");
}

@NotNull
private static TimestampSampler createTimestampSampler(long interval, char timeUnit, int position) throws SqlException {
public static TimestampSampler getInstance(long interval, char timeUnit, int position) throws SqlException {
switch (timeUnit) {
case 'U':
// micros
Expand Down Expand Up @@ -131,4 +121,46 @@ private static TimestampSampler createTimestampSampler(long interval, char timeU

}
}

/**
* Parses strings such as '10m', '3M', '5d', '12h', 'y', '35s'
*
* @param cs the key
* @param position position in SQL text to report error against
* @return instance of appropriate TimestampSampler
* @throws SqlException when input string is invalid
*/
public static TimestampSampler getInstance(CharSequence cs, int position) throws SqlException {
int k = findIntervalEndIndex(cs, position);
assert cs.length() > k;

long n = parseInterval(cs, k, position);
return getInstance(n, cs.charAt(k), position + k);
}

/**
* Parse interval value from string. Expected to be called after {@link #findIntervalEndIndex(CharSequence, int)}
* has been called and returned a valid index. Behavior is undefined if called with invalid index.
*
* @param cs token to parse interval from
* @param intervalEnd end of interval token, exclusive
* @param position position in SQL text to report error against
* @return parsed interval value
* @throws SqlException when input string is invalid
*/
public static long parseInterval(CharSequence cs, int intervalEnd, int position) throws SqlException {
if (intervalEnd == 0) {
// 'SAMPLE BY m' is the same as 'SAMPLE BY 1m' etc.
return 1;
}
try {
int n = Numbers.parseInt(cs, 0, intervalEnd);
if (n == 0) {
throw SqlException.$(position, "zero is not a valid sample value");
}
return n;
} catch (NumericException e) {
throw SqlException.$(position, "invalid sample value [value=").put(cs).put(']');
}
}
}
Loading

0 comments on commit 318c2be

Please sign in to comment.