_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

• 📝 Generate docstrings

core/src/test/java/io/questdb/test/griffin/engine/functions/array/DoubleArrayRoundFunctionFactoryTest.java (1)

38-271: Text block refactoring looks good, but appears unrelated to this PR's purpose.

This file tests the round() function for double arrays, which doesn't appear related to the arg_min/arg_max aggregate functions described in the PR objectives. The text block refactoring improves readability and is correctly implemented, but including unrelated formatting changes in a feature PR can make reviews more difficult and muddies the git history.

Consider keeping such refactoring separate from feature additions in future PRs to maintain clear separation of concerns.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ArgMaxTimestampDoubleGroupByFunctionFactoryTest.java (3)

171-177: Self-comparison in parallel test doesn't verify correctness.

assertSqlCursors is called with the same SQL for both expected and actual parameters. This only verifies that the query executes without error in parallel mode, but doesn't actually validate the result correctness.

Consider comparing against a sequential execution or a known-correct baseline:

💡 Suggested approach

-                        TestUtils.assertSqlCursors(
-                                engine,
-                                sqlExecutionContext,
-                                sql,
-                                sql,
-                                LOG
-                        );
+                        // Compare parallel execution result against sequential baseline
+                        String sequentialSql = "select sym, arg_max(value, key) from tab group by sym order by sym";
+                        // Run with single worker context for comparison, or use assertSql with expected values
+                        TestUtils.assertSqlCursors(
+                                engine,
+                                sqlExecutionContext,
+                                sequentialSql,  // expected (could use a single-threaded context)
+                                sql,            // actual (parallel)
+                                LOG
+                        );

Alternatively, compute expected results using a subquery or CTE that doesn't rely on arg_max, then compare.

---

200-206: Same self-comparison issue as noted above.

This test also compares the SQL query to itself. Consider applying the same fix to verify parallel results against a sequential or computed baseline.

---

229-235: Same self-comparison issue as noted above.

This test also compares the SQL query to itself.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ArgMaxTimestampUuidGroupByFunctionFactoryTest.java (1)

57-66: Consider strengthening parallel test assertions.

The pattern TestUtils.assertSqlCursors(engine, sqlExecutionContext, sql, sql, LOG) compares a query against itself. While this verifies the query executes without error and produces consistent results, it doesn't validate that the result is actually correct. For non-deterministic data (like rnd_uuid4()), this may be acceptable for testing parallel consistency, but it won't catch logic errors in the aggregation.

Consider adding at least one parallel test with deterministic data where the expected result can be explicitly verified, similar to the pattern used in testArgMaxSimple.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ArgMaxDoubleTimestampGroupByFunctionFactoryTest.java (1)

185-212: Consider clarifying test intent.

The timestamp_sequence(0, 1000) in the CASE expression always restarts from 0 for each row where x > 1000000, rather than continuing from the previous sequence. This still exercises the merge-null-dest-valid-src scenario, but the actual timestamp values will all be 0 for rows with valid keys.

This is likely intentional to test the merge logic (not the timestamp ordering), but a brief inline comment would clarify.

core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/ArgMinDoubleTimestampGroupByFunctionFactoryTest.java (1)

57-66: Consider adding a verification against a known result.

The assertSqlCursors(sql, sql) pattern validates determinism (same query returns same results) but doesn't verify correctness against a reference implementation. This is acceptable for large-scale parallel testing where manual expected values are impractical, but consider adding at least one parallel test with a smaller dataset that asserts specific expected values.

• X
• Mastodon
• Reddit
• LinkedIn

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.