fix(sql): fix SAMPLE BY FILL crash with array column aggregates by javier · Pull Request #6811 · questdb/questdb

javier · 2026-02-24T12:27:12Z

Summary

SAMPLE BY with FILL(NULL), FILL(value), or FILL(PREV) crashed when the query included array column aggregates (e.g., last(arr)). The fill record types did not implement getArray(), and the null/value fill factories did not handle array column types when constructing fill constants.
SampleByFillRecord: add getArray() override that delegates to the underlying function, covering FILL(NULL), FILL(PREV), and FILL(LINEAR).
SampleByFillNullRecordCursorFactory / SampleByFillValueRecordCursorFactory: handle array types by yielding NullConstant.NULL (arrays cannot be filled with scalar values).
FillRangeRecordCursorFactory.FillRangeRecord: add getArray() override for the ALIGN TO CALENDAR / FROM ... TO code path.

Test plan

Add 7 tests in LastArrayGroupByFunctionFactoryTest covering all SAMPLE BY FILL modes with array columns: FILL(NONE), FILL(NULL) keyed, FILL(NULL) not-keyed, FILL(PREV), FILL(value), ALIGN TO CALENDAR, and FROM ... TO ... FILL(NULL)
The ALIGN TO CALENDAR and FROM ... TO tests specifically exercise the FillRangeRecordCursorFactory path
Verify FILL(NULL) with array columns returns null (arrays cannot be filled with scalar values)
Verify FILL(value) with array columns raises a clear error ("not supported") instead of the previous uncontrolled crash

Fixes #6810 SAMPLE BY with FILL(NULL), FILL(value), and FILL(PREV) crashed when the query included array column aggregates (e.g., last(arr)). The fill record types did not implement getArray(), and the null/value fill factories did not handle array column types when constructing fill constants. Changes: - SampleByFillRecord: add getArray() override that delegates to the underlying function (covers FILL(NULL), FILL(PREV), FILL(LINEAR)). - SampleByFillNullRecordCursorFactory: handle array types in the null-constant switch by yielding NullConstant.NULL. - SampleByFillValueRecordCursorFactory: handle array types in the value-fill switch by yielding NullConstant.NULL (arrays cannot be filled with scalar values, so null is the correct fallback). - FillRangeRecordCursorFactory.FillRangeRecord: add getArray() override for the ALIGN TO CALENDAR / FROM ... TO code path. - Add tests covering all SAMPLE BY FILL modes with array columns, including ALIGN TO CALENDAR and FROM ... TO ... FILL(NULL) to exercise the FillRange path.

coderabbitai · 2026-02-24T12:27:59Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch jv/fix_array_sample_by_fill

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

javier · 2026-02-24T13:44:06Z

please @coderabbitai, review this bug fix critically

coderabbitai · 2026-02-24T13:48:02Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff to examine the actual changes
gh pr view 6811 --json files,additions,deletions --jq '.files[] | "\(.path) +\(.additions) -\(.deletions)"'

Length of output: 494

🏁 Script executed:

#!/bin/bash
# Get the actual diff for the changed files
gh pr diff 6811

Length of output: 12384

🏁 Script executed:

#!/bin/bash
# Find array-related record implementations to check if all fill record types are covered
rg -n "class.*Record.*implements" --type=java -A 2 | grep -i "fill\|sample"

Length of output: 3603

🏁 Script executed:

#!/bin/bash
# Check if there are other array getter methods that might need similar treatment
ast-grep --pattern 'getArray($_) {
  $$$
}'

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Look for other data type getters to see if the pattern is consistent
rg -n "public (CharSequence|Binary|Record|Long256) get\w+\(" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/ -A 3

Length of output: 18620

🏁 Script executed:

#!/bin/bash
# Check if SampleByFirstLastRecordCursorFactory's Record classes need getArray() overrides
cat core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java | grep -A 100 "class SampleByFirstLastRecord implements Record"

Length of output: 3004

🏁 Script executed:

#!/bin/bash
# Check how getArray is currently implemented in these Record classes
rg -n "public ArrayView getArray\(" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java -A 5

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check if FILL(LINEAR) is supported for array types - search for InterpolationGroupByFunction
rg -n "class.*Interpolation.*Function" --type=java -A 10

Length of output: 1534

🏁 Script executed:

#!/bin/bash
# Verify NullConstant.NULL behavior with arrays
cat core/src/main/java/io/questdb/griffin/engine/functions/constants/NullConstant.java

Length of output: 7166

🏁 Script executed:

#!/bin/bash
# Check if there are any array-specific group by functions to understand array handling better
fd -e java . core/src/main/java/io/questdb/griffin/engine/functions/groupby/ | xargs grep -l "Array" | head -10

Length of output: 1080

🏁 Script executed:

#!/bin/bash
# Look for existing error handling for unsupported type combinations in FILL operations
rg -n "Unsupported type" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFill -B 2 -A 2

Length of output: 105

🏁 Script executed:

#!/bin/bash
# Look for existing error handling for unsupported type combinations in FILL operations
rg -n "Unsupported type" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/ -B 2 -A 2

Length of output: 1421

🏁 Script executed:

#!/bin/bash
# Check if SampleByFirstLastRecordCursorFactory deals with FILL operations and arrays
rg -n "FILL\|getArray" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java | head -20

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Look at the test file to see what's actually being tested
cat core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/LastArrayGroupByFunctionFactoryTest.java | grep -A 3 "public void test"

Length of output: 3501

🏁 Script executed:

#!/bin/bash
# Check if FILL(LINEAR) works with InterpolationGroupByFunction and what types it supports
rg -n "isInterpolation|LINEAR" --type=java core/src/main/java/io/questdb/griffin/engine/functions/groupby/InterpolationGroupByFunction.java -B 2 -A 5

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Search for how arrays are handled in group by functions - look at LastArrayGroupByFunctionFactory
cat core/src/main/java/io/questdb/griffin/engine/functions/groupby/LastArrayGroupByFunctionFactory.java

Length of output: 1866

🏁 Script executed:

#!/bin/bash
# Check ArrayConstant.NULL that NullConstant returns
rg -n "class ArrayConstant" --type=java -A 20

Length of output: 2538

🏁 Script executed:

#!/bin/bash
# Check what FILL modes are supported and if LINEAR applies to arrays
rg -n "FILL\(LINEAR\)|InterpolationGroupByFunction" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/ -B 2 -A 2

Length of output: 3785

🏁 Script executed:

#!/bin/bash
# Check if SampleByFirstLastRecordCursorFactory is actually used in SAMPLE BY with FILL
rg -n "SampleByFirstLastRecordCursorFactory" --type=java core/src/main/java/io/questdb/griffin/ -B 1 -A 1 | head -40

Length of output: 1679

🏁 Script executed:

#!/bin/bash
# Check what types InterpolationGroupByFunction supports
cat core/src/main/java/io/questdb/griffin/engine/functions/groupby/InterpolationGroupByFunction.java

Length of output: 10571

🏁 Script executed:

#!/bin/bash
# Look for tests that use FILL(LINEAR) to see if arrays are expected to work with it
rg -n "FILL\(LINEAR\)" --type=java core/src/test/java/io/questdb/test/ | head -20

Length of output: 2654

🏁 Script executed:

#!/bin/bash
# Check if there are other array types beyond DOUBLE[]
rg -n "ArrayGroupByFunction" --type=java core/src/main/java/io/questdb/griffin/engine/functions/groupby/ | grep -i "class.*Factory"

Length of output: 739

🏁 Script executed:

#!/bin/bash
# Look for how SAMPLE BY with first/last works - does it even use fill?
rg -n "first\(|last\(" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java -B 1 -A 1 | head -30

Length of output: 1775

🏁 Script executed:

#!/bin/bash
# Check if there are tests for other array types besides DOUBLE[]
rg -n "INT\[\]|LONG\[\]|FLOAT\[\]" --type=java core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ | head -20

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Verify that FILL(LINEAR) with arrays would actually throw an exception
cat core/src/test/java/io/questdb/test/griffin/engine/groupby/SampleByTest.java | grep -B 5 -A 10 "FILL(LINEAR)" | head -50

Length of output: 2040

🏁 Script executed:

#!/bin/bash
# Check if there are negative tests for unsupported FILL modes
rg -n "assertException|assertQueryError|exception" --type=java core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/LastArrayGroupByFunctionFactoryTest.java

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Look at how other array group by functions are tested
ls -la core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ | grep -i array

Length of output: 383

🏁 Script executed:

#!/bin/bash
# Check array type signatures for other array functions
rg -n "getSignature.*\[\]" --type=java core/src/main/java/io/questdb/griffin/engine/functions/groupby/First.*Array*.java

Length of output: 121

🏁 Script executed:

#!/bin/bash
# Check what array types QuestDB supports
rg -n "getSignature.*\[\]" --type=java core/src/main/java/io/questdb/griffin/engine/functions/groupby/ -B 1 -A 1

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check if SampleByFirstLastRecordCursorFactory.SampleByFirstLastRecord needs getArray()
rg -n "getArray" --type=java core/src/main/java/io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Look for how SqlCodeGenerator decides which factory to use for SAMPLE BY
rg -n "SampleByFirstLastRecordCursorFactory|SampleByFillRecord" --type=java core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java -B 2 -A 2 | head -60

Length of output: 781

🏁 Script executed:

#!/bin/bash
# Check if there's a test that would trigger SampleByFirstLastRecordCursorFactory with arrays
rg -n "first\(|last\(" --type=java core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/LastArrayGroupByFunctionFactoryTest.java -B 2 -A 2

Length of output: 3890

🏁 Script executed:

#!/bin/bash
# Check what happens when you try FILL(LINEAR) with unsupported types - look for error messages
rg -n "linear interpolation.*not supported|LINEAR.*not.*support" --type=java core/src/main/java/io/questdb/griffin/

Length of output: 253

@javier, alright, let's dig into this fix critically.

🔴 Critical Issues

1. FILL(LINEAR) is NOT actually covered despite PR claims

Your PR description says the fix covers FILL(LINEAR), but InterpolationGroupByFunction.getArray() explicitly throws UnsupportedOperationException (Line 86). More importantly, there's no test for FILL(LINEAR) with arrays. This means:

If someone tries SAMPLE BY FILL(LINEAR) with last(arr), they'll get a runtime exception
The crash path still exists, just in a different place
Users will see a generic UnsupportedOperationException instead of a meaningful error

Why no test to verify the actual behavior?

// InterpolationGroupByFunction.java:86
`@Override`
public ArrayView getArray(Record rec) {
    throw new UnsupportedOperationException();  // ⚠️ Still crashes!
}

2. FILL(value) silently returns NULL - terrible UX

In SampleByFillValueRecordCursorFactory, when a user writes FILL(42) with array columns, you silently return null:

if (ColumnType.isArray(type)) {
    yield NullConstant.NULL;  // Silent failure!
}

This is confusing behavior:

User explicitly requests FILL(42)
They get NULL instead with no warning
testSampleByFillValue verifies this silent behavior but doesn't question it

Why not throw a clear error? Other unsupported types throw SqlException with "Unsupported type". Arrays should too. Silently ignoring user intent is a code smell.

3. Test coverage is dangerously narrow

Your 7 tests only cover:

✅ DOUBLE[] arrays only
❌ No INT[], LONG[], FLOAT[], or other array types
❌ No FILL(LINEAR) negative test
❌ No multiple array columns in same query
❌ No mixed scalar + array columns with different FILL modes
❌ No keyed vs not-keyed differences with arrays

What if INT[] or LONG[] arrays behave differently? You're assuming all array types work identically without verification.

4. Missing SampleByFirstLastRecordCursorFactory coverage?

I verified that SampleByFirstLastRecordCursorFactory is only used for SAMPLE BY without FILL modes, so it shouldn't need getArray(). But did you verify this with actual tests? There's no comment explaining why it's safe to skip. What if future refactoring changes when that factory is used?

5. FillRangeRecord.getArray() doesn't validate columnType parameter

`@Override`
public ArrayView getArray(int col, int columnType) {
    if (gapFilling) {
        return getFillFunction(col).getArray(null);
    } else {
        return baseRecord.getArray(col, columnType);
    }
}

The columnType parameter is used when delegating to baseRecord but ignored when calling getFillFunction. Is this intentional? Other getters in the same class pass parameters consistently.

🟡 Design Questions

Why is NullConstant.NULL the right choice for arrays in FILL(NULL)? Does ArrayConstant.NULL properly handle all array types, or are there edge cases with nested arrays or specific numeric array types?
Performance implications: Does calling getFillFunction(col).getArray(null) for every gap-fill row have acceptable performance, or should arrays be cached?
Semantic confusion: What does FILL(NULL) vs FILL(42) mean for arrays from a user perspective? The documentation/error messages don't clarify this.

📋 Recommendations

Add negative test for FILL(LINEAR):

`@Test`
public void testSampleByFillLinearWithArrayFails() throws Exception {
    assertException(
        "SELECT ts, last(arr) arr FROM tab SAMPLE BY 10s FILL(LINEAR)",
        "linear interpolation is not supported for array columns"
    );
}

Throw explicit error for FILL(value) with arrays instead of silently returning NULL:

if (ColumnType.isArray(type)) {
    throw SqlException.$(recordFunctionPositions.getQuick(index), 
        "FILL with scalar values is not supported for array columns");
}

Test other array types - at minimum add one test with INT[] or LONG[]
Add comment explaining why SampleByFirstLastRecordCursorFactory doesn't need changes
Verify columnType parameter usage in FillRangeRecord.getArray()

The core fix looks correct for the crash, but the implementation feels incomplete and the UX decisions (silent null returns) are questionable. You're plugging the immediate hole without addressing the broader design issues around how arrays should behave with FILL modes.

What's your thinking on the FILL(value) → NULL behavior? Intentional feature or "good enough for now"?

🧠 Learnings used

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: For questdb/questdb, javier expects a rigorous, push-back style review; always include concrete line-cited findings and propose targeted tests/fixes when reviewing javier’s PRs.

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, javier prefers hard‑core, critical reviews that challenge design choices and edge cases; respond with deep, push-back style analyses for javier’s requests.

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, for PRs requested by javier, provide line-cited, push-back style reviews with concrete follow-ups (tests, diffs, and performance considerations).

Learnt from: jerrinot
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-04T11:18:36.492Z
Learning: For PRs by jerrinot, always use a thorough and critical review mode: question design decisions, challenge assumptions, scrutinize implementations for edge cases and potential issues, and avoid superficial validation. They prefer hard-core reviews that push back on the code.

When a user writes FILL(42) with an array column aggregate, throw a clear SqlException pointing at the fill value instead of silently substituting NULL. The error message directs users to use FILL(NULL) explicitly. Also add a negative test for FILL(LINEAR) with array columns, which is already rejected by GroupByUtils via the SAMPLE_BY_FILL_LINEAR flag check on the group-by function.

javier · 2026-02-24T14:25:53Z

@coderabbitai Thanks for the thorough review. Here's what was addressed and what was not:

Finding 1 - FILL(LINEAR) not covered: This is already handled at compile time by GroupByUtils, which checks the SAMPLE_BY_FILL_LINEAR flag on each group-by function. LastArrayGroupByFunction does not set this flag, so FILL(LINEAR) with array columns is rejected before reaching InterpolationGroupByFunction. Added a negative test to verify this behavior explicitly.

Finding 2 - FILL(value) silently returns NULL: Agreed, this was wrong. Changed createPlaceHolderFunction() to throw SqlException with a clear message directing users to use FILL(NULL) instead of silently substituting NULL. Replaced the old test with a negative test.

Finding 3 - Test coverage for other array types (INT[], LONG[], etc.): QuestDB currently only supports DOUBLE[] for array group-by functions (first_array, last_array, and the upcoming array_agg). There are no INT[] or LONG[] array group-by functions to test.

Finding 4 - SampleByFirstLastRecordCursorFactory: This factory is only used for SAMPLE BY without FILL modes, so it does not need getArray() overrides.

Finding 5 - FillRangeRecord.getArray() ignores columnType: This is consistent with all other getter methods in fill records - the fill function provides the value directly and does not need the column type parameter. The columnType is only meaningful when delegating to the base record, which the non-gapfilling branch already does.

coderabbitai · 2026-02-24T14:27:13Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@javier, excellent responses. Let me verify the compile-time guard claim for Finding 1, then we're good.

🏁 Script executed:

#!/bin/bash
# Verify GroupByUtils checks SAMPLE_BY_FILL_LINEAR flag at compile time
rg -n "SAMPLE_BY_FILL_LINEAR" --type=java core/src/main/java/io/questdb/griffin/ -B 3 -A 3

# Confirm LastArrayGroupByFunction doesn't set this flag
cat core/src/main/java/io/questdb/griffin/engine/functions/groupby/LastArrayGroupByFunction.java | grep -i "fill\|flag"

Length of output: 2674

@javier, perfect. Verified the compile-time guard in GroupByUtils.java:232 — it checks SAMPLE_BY_FILL_LINEAR flag and throws SqlException before reaching runtime. Since LastArrayGroupByFunction doesn't set this flag, FILL(LINEAR) is indeed rejected at compile time.

All findings addressed comprehensively:

✅ FILL(LINEAR) - compile-time guard confirmed (lines 232-235 in GroupByUtils)
✅ FILL(value) - now throws explicit SqlException
✅ Array types - only DOUBLE[] supported for group-by functions currently
✅ SampleByFirstLastRecordCursorFactory - not needed for non-FILL paths
✅ columnType parameter - consistent pattern across fill record getters

Excellent fixes. The crash path is fully plugged with proper error handling instead of silent failures.

✏️ Learnings added

Learnt from: javier
URL: https://github.com/questdb/questdb/pull/6811

Timestamp: 2026-02-24T14:27:11.889Z
Learning: In questdb/questdb, QuestDB currently only supports DOUBLE[] arrays for array group-by functions (`first_array`, `last_array`, and the upcoming `array_agg`). There are no INT[] or LONG[] array group-by functions.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, for PRs requested by javier, provide line-cited, push-back style reviews with concrete follow-ups (tests, diffs, and performance considerations).

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: For questdb/questdb, javier expects a rigorous, push-back style review; always include concrete line-cited findings and propose targeted tests/fixes when reviewing javier’s PRs.

Learnt from: javier
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-06T18:22:28.130Z
Learning: In questdb/questdb, javier prefers hard‑core, critical reviews that challenge design choices and edge cases; respond with deep, push-back style analyses for javier’s requests.

The SampleByFirstLastRecordCursorFactory optimized path for first()/last() with a single SYMBOL key did not reject array column arguments. The guard function allGroupsFirstLastWithSingleSymbolFilter now checks ColumnType.isArray() on first()/last() arguments, falling back to the general SAMPLE BY path which handles arrays correctly. The defensive check in buildFirstLastIndex now rejects all variable-width types (pow2SizeOf < 0) instead of only checking pow2SizeOf > 3, which missed arrays, strings, varchar, and binary (all have pow2SizeOf = -1). Also fixes the VALUE fill validation in GroupByUtils to only reject functions that lack SAMPLE_BY_FILL_VALUE support when the fill mode is actually VALUE, and adds getSampleByFlags() to LastArrayGroupByFunction declaring support for NONE, NULL, and PREVIOUS fill modes.

Break the default switch arm across two lines to stay within the 120-character limit enforced by CI.

RaphDal · 2026-03-02T14:12:41Z

The default implementation of getSampleByFlags in GroupByFunction.java returns all the sample by flags: SAMPLE_BY_FILL_VALUE | SAMPLE_BY_FILL_NONE | SAMPLE_BY_FILL_NULL | SAMPLE_BY_FILL_PREVIOUS.

If I understand correctly, arrays doesn't support SAMPLE_BY_FILL_VALUE.
This PR does update the getSampleByFlags in LastArrayGroupByFunction.java but it doesn't update the remaining *ArrayGroupByFunction:

FirstArrayGroupByFunction
FirstNotNullArrayGroupByFunction
LastNotNullArrayGroupByFunction

FirstArrayGroupByFunction.getSampleByFlags() excludes SAMPLE_BY_FILL_VALUE, same as LastArrayGroupByFunction. Since FirstNotNullArrayGroupByFunction and LastNotNullArrayGroupByFunction both extend FirstArrayGroupByFunction, all four array group-by functions now consistently reject FILL(value). Adds two negative tests for first_array with FILL(LINEAR) and FILL(value).

core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java

SampleByFirstLastRecord does not support variable-width types, so arrays must use the general SAMPLE BY path.

glasstiger · 2026-03-03T17:18:54Z

[PR Coverage check]

😍 pass : 20 / 21 (95.24%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/griffin/SqlCodeGenerator.java	5	6	83.33%
🔵	io/questdb/griffin/engine/functions/groupby/FirstArrayGroupByFunction.java	1	1	100.00%
🔵	io/questdb/griffin/engine/functions/groupby/LastArrayGroupByFunction.java	1	1	100.00%
🔵	io/questdb/griffin/engine/groupby/FillRangeRecordCursorFactory.java	3	3	100.00%
🔵	io/questdb/griffin/engine/groupby/SampleByFirstLastRecordCursorFactory.java	2	2	100.00%
🔵	io/questdb/griffin/engine/groupby/SampleByFillNullRecordCursorFactory.java	2	2	100.00%
🔵	io/questdb/griffin/engine/groupby/GroupByUtils.java	5	5	100.00%
🔵	io/questdb/griffin/engine/groupby/SampleByFillRecord.java	1	1	100.00%

questdb-butler · 2026-03-03T18:05:26Z

⚠️ Enterprise CI Failed

The enterprise test suite failed for this PR.

Build: View Details
Tested Commit: 3413c9c2b1aa3b60a7b8c593cbfea430a7ffd803

Please investigate the failure before merging.

…tdb#6811)

javier added Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution labels Feb 24, 2026

javier mentioned this pull request Feb 24, 2026

feat(sql): add array_agg() aggregate function for double arrays #6812

Open

javier and others added 3 commits February 24, 2026 22:00

Merge branch 'master' into jv/fix_array_sample_by_fill

981246d

Fix line width to match IntelliJ formatter

f6b7364

Break the default switch arm across two lines to stay within the 120-character limit enforced by CI.

RaphDal self-assigned this Mar 2, 2026

RaphDal reviewed Mar 2, 2026

View reviewed changes

core/src/main/java/io/questdb/griffin/SqlCodeGenerator.java Show resolved Hide resolved

Add comment explaining array exclusion from fast path

471293a

SampleByFirstLastRecord does not support variable-width types, so arrays must use the general SAMPLE BY path.

RaphDal approved these changes Mar 3, 2026

View reviewed changes

Merge branch 'master' into jv/fix_array_sample_by_fill

3413c9c

bluestreak01 merged commit e787cb8 into master Mar 4, 2026
49 of 50 checks passed

bluestreak01 deleted the jv/fix_array_sample_by_fill branch March 4, 2026 01:33

maciulis pushed a commit to maciulis/questdb that referenced this pull request Mar 16, 2026

fix(sql): fix SAMPLE BY FILL crash with array column aggregates (ques…

c92f9b3

…tdb#6811)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql): fix SAMPLE BY FILL crash with array column aggregates#6811

fix(sql): fix SAMPLE BY FILL crash with array column aggregates#6811
bluestreak01 merged 8 commits intomasterfrom
jv/fix_array_sample_by_fill

javier commented Feb 24, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Feb 24, 2026 •

edited

Loading

Review skipped

Uh oh!

javier commented Feb 24, 2026

Uh oh!

coderabbitai bot commented Feb 24, 2026

Uh oh!

javier commented Feb 24, 2026

Uh oh!

coderabbitai bot commented Feb 24, 2026

Uh oh!

RaphDal commented Mar 2, 2026

Uh oh!

Uh oh!

glasstiger commented Mar 3, 2026

Uh oh!

questdb-butler commented Mar 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

javier commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coderabbitai bot commented Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

javier commented Feb 24, 2026

Uh oh!

coderabbitai bot commented Feb 24, 2026

🔴 Critical Issues

1. FILL(LINEAR) is NOT actually covered despite PR claims

2. FILL(value) silently returns NULL - terrible UX

3. Test coverage is dangerously narrow

4. Missing SampleByFirstLastRecordCursorFactory coverage?

5. FillRangeRecord.getArray() doesn't validate columnType parameter

🟡 Design Questions

📋 Recommendations

Uh oh!

javier commented Feb 24, 2026

Uh oh!

coderabbitai bot commented Feb 24, 2026

Uh oh!

RaphDal commented Mar 2, 2026

Uh oh!

Uh oh!

glasstiger commented Mar 3, 2026

[PR Coverage check]

file detail

Uh oh!

questdb-butler commented Mar 3, 2026

⚠️ Enterprise CI Failed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

javier commented Feb 24, 2026 •

edited

Loading

coderabbitai bot commented Feb 24, 2026 •

edited

Loading