core/src/main/java/io/questdb/griffin/engine/functions/math/ExpDoubleFunctionFactory.java (1)

49-49: function field should be private.

The field is package-private, but since ExpFunction is a private static inner class there is no defensive need for package-level visibility. Making it private is more idiomatic.

♻️ Proposed fix

-    final Function function;
+    private final Function function;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/math/ExpDoubleFunctionFactory.java`
at line 49, The field "function" in ExpDoubleFunctionFactory is package-private
but should be private; update the declaration of the field named function in
class ExpDoubleFunctionFactory to be private (i.e., change its visibility to
private final) to match the private static inner class ExpFunction and follow
idiomatic encapsulation.

core/src/main/java/io/questdb/griffin/engine/functions/math/SqrtDoubleFunctionFactory.java (1)

48-70: All arithmetic unary functions uniformly use the default complexity from ArithmeticUnaryFunction.

The concern about sqrt being a transcendental/expensive operation is architecturally valid, but this applies to all transcendental math functions (log, exp, sin, cos, tan, asin, acos, atan), not just sqrt. None of them override getComplexity(). Since Function.COMPLEXITY_ARITHMETIC = 2 is uniformly applied across all arithmetic unary operations via the interface default, this appears to be an intentional design choice. If complexity stratification for transcendental operations is desired, it would need to be addressed across all such functions consistently, not on a per-function basis.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/math/SqrtDoubleFunctionFactory.java`
around lines 48 - 70, The review points out that overriding complexity only in
SqrtFunction is inconsistent; either keep the current uniform default from
ArithmeticUnaryFunction or implement a consistent strategy for all
transcendental ops. Fix by removing any per-function getComplexity override in
SqrtFunction (leave SqrtFunction as shown using the ArithmeticUnaryFunction
default) or, if you want higher complexity for transcendental ops, introduce a
new marker/interface (e.g., TranscendentalUnaryFunction or change
ArithmeticUnaryFunction default) and update all transcendental implementations
(SqrtFunction, log/exp/sin/cos/tan/asin/acos/atan classes) to implement that so
complexity is applied consistently across those functions (refer to
SqrtFunction, ArithmeticUnaryFunction, and getComplexity).

core/src/main/java/io/questdb/cairo/sql/Function.java (1)

138-140: getComplexity() is placed out of alphabetical order.

Per the coding guideline, instance methods should be sorted alphabetically. getComplexity() (line 138) is placed before getArray() (line 148), but alphabetically getArray < getComplexity. It should be between getChar() and getDate().

Move getComplexity() to its correct alphabetical position

Move the getComplexity() default method to between getChar(Record) (currently around line 158) and getDate(Record) (currently around line 160).

As per coding guidelines: "Java class members must be grouped by kind (static vs. instance) and visibility, and sorted alphabetically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/io/questdb/cairo/sql/Function.java` around lines 138 -
140, Move the default instance method getComplexity() so instance methods remain
alphabetically ordered: remove the existing getComplexity() definition and
reinsert it between getChar(Record) and getDate(Record) in the Function
interface; ensure the method signature and return value (return
COMPLEXITY_COLUMN) are unchanged and that imports/formatting remain consistent.

core/src/main/java/io/questdb/griffin/engine/functions/str/SubStringFunctionFactory.java (1)

94-97: getComplexity() should follow getCenter() alphabetically.

getCenter (line 100) sorts before getComplexity lexicographically ("getCe" < "getCo"), but getComplexity is placed first. Swap the order to maintain the alphabetical convention.

Reorder methods

-        `@Override`
-        public int getComplexity() {
-            return Function.addComplexity(COMPLEXITY_STRING_OP, TernaryFunction.super.getComplexity());
-        }
-
         `@Override`
         public Function getCenter() {
             return startFunc;
         }

+        `@Override`
+        public int getComplexity() {
+            return Function.addComplexity(COMPLEXITY_STRING_OP, TernaryFunction.super.getComplexity());
+        }
+
         `@Override`
         public Function getLeft() {

As per coding guidelines: "Java class members must be grouped by kind (static vs. instance) and visibility, and sorted alphabetically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/str/SubStringFunctionFactory.java`
around lines 94 - 97, In SubStringFunctionFactory, reorder the instance methods
so that getCenter() appears before getComplexity() to match alphabetical
ordering of method names; locate the two methods named getCenter and
getComplexity in the SubStringFunctionFactory class and swap their positions
(keeping their implementations unchanged) so getCenter() is declared prior to
getComplexity().

core/src/main/java/io/questdb/griffin/engine/functions/CursorFunction.java (1)

54-57: getComplexity() placed before getArray() — same alphabetical ordering issue.

Same pattern as in Function.java: getComplexity() should be positioned between getChar() and getDate() to maintain alphabetical ordering.

As per coding guidelines: "Java class members must be grouped by kind (static vs. instance) and visibility, and sorted alphabetically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/main/java/io/questdb/griffin/engine/functions/CursorFunction.java`
around lines 54 - 57, The getComplexity() method in CursorFunction is out of
alphabetical order — move the public int getComplexity() { return
COMPLEXITY_SUBQUERY; } method so it sits after getChar() and before getDate()
(i.e., between getChar() and getDate()) to comply with the class members
grouping/sorting rule; ensure visibility and static/instance grouping remain
unchanged when relocating the method.

core/src/main/java/io/questdb/griffin/engine/functions/str/ReplaceStrFunctionFactory.java (1)

96-99: getComplexity() should follow getCenter() alphabetically.

Same issue as in SubStringFunctionFactory: getCenter sorts before getComplexity lexicographically. Swap the order.

Reorder methods

-        `@Override`
-        public int getComplexity() {
-            return Function.addComplexity(COMPLEXITY_STRING_OP, TernaryFunction.super.getComplexity());
-        }
-
         `@Override`
         public Function getCenter() {
             return oldSubStr;
         }

+        `@Override`
+        public int getComplexity() {
+            return Function.addComplexity(COMPLEXITY_STRING_OP, TernaryFunction.super.getComplexity());
+        }
+
         `@Override`
         public Function getLeft() {

As per coding guidelines: "Java class members must be grouped by kind (static vs. instance) and visibility, and sorted alphabetically."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/str/ReplaceStrFunctionFactory.java`
around lines 96 - 99, Move the getComplexity() method so it appears after
getCenter() within the ReplaceStrFunctionFactory class to satisfy alphabetical
ordering of instance methods; ensure you retain the existing implementation
(returning Function.addComplexity(COMPLEXITY_STRING_OP,
TernaryFunction.super.getComplexity())) and keep method visibility unchanged so
grouping and sorting rules for class members are preserved.

core/src/main/java/io/questdb/griffin/engine/functions/math/BitwiseAndLongFunctionFactory.java (1)

55-85: Semantic concern valid, but implementation is intentionally consistent across all bitwise operations.

Bitwise AND is indeed a logical/bitwise operation, not an arithmetic one—this semantic distinction is correct. However, the codebase shows that this design is intentional and applied consistently: all bitwise operations (&, |, ^ for Long/Int/IPv4 variants, and ~ for unary) implement ArithmeticBinaryFunction or ArithmeticUnaryFunction.

ArithmeticBinaryFunction adds COMPLEXITY_ARITHMETIC to the cost model, grouping bitwise operations with arithmetic ones based on comparable computational cost rather than semantic category. The suggested BitwiseBinaryFunction sub-interface does not exist in the codebase, and creating one is not necessary for consistency—all bitwise operations already follow the same pattern.

If the complexity-based grouping is the intended design, the naming remains semantically imprecise, but this is a codebase-wide pattern, not an issue specific to this change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/math/BitwiseAndLongFunctionFactory.java`
around lines 55 - 85, The implementation intentionally uses
ArithmeticBinaryFunction for bitwise ops; update BitAndLongFunction (and similar
bitwise classes) to add a brief clarifying comment explaining that bitwise
operations (e.g., BitAndLongFunction) are grouped under ArithmeticBinaryFunction
to reflect complexity-based costing (COMPLEXITY_ARITHMETIC) rather than semantic
arithmetic, so future maintainers understand this naming/design choice and don't
introduce a BitwiseBinaryFunction; place the comment near the class declaration
and mention ArithmeticBinaryFunction and COMPLEXITY_ARITHMETIC.

core/src/main/java/io/questdb/griffin/engine/functions/conditional/CaseFunction.java (1)

32-35: Consider adding a named constant for the CASE complexity value.

CaseFunction uses literal 5 here, but all other functions in the codebase (ReplaceStrFunction, SubStringFunction, MatchStrFunction, ArithmeticUnaryFunction, etc.) use named constants like COMPLEXITY_REGEX, COMPLEXITY_STRING_OP, and COMPLEXITY_ARITHMETIC. Define a constant like COMPLEXITY_CASE = 5 in the Function interface to maintain consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/conditional/CaseFunction.java`
around lines 32 - 35, The CaseFunction.getComplexity implementation uses a magic
literal 5; add a named constant COMPLEXITY_CASE = 5 to the Function interface
and replace the literal in CaseFunction with Function.COMPLEXITY_CASE when
calling Function.addComplexity(Function.COMPLEXITY_CASE,
MultiArgFunction.super.getComplexity()); update any imports or references if
necessary so CaseFunction, Function.addComplexity, and
MultiArgFunction.super.getComplexity() compile against the new constant.

core/src/main/java/io/questdb/griffin/engine/functions/geo/GeoDistanceMetersFunctionFactory.java (1)

194-197: Child complexity is not propagated in specialized geo-distance functions.

ConstPoint1GeoDistanceFunction and GeoDistanceFunction extend DoubleFunction and manually manage child functions (lat2Func, lon2Func, etc.) without implementing arity interfaces like BinaryFunction or UnaryFunction. Their getComplexity() overrides return a flat COMPLEXITY_GEO (30) without aggregating child complexity. This design diverges from the aggregation pattern used by arity interfaces, where child complexity is summed via Function.addComplexity().

The practical impact is minimal because COMPLEXITY_GEO exceeds the materialization threshold (3), so the sort key will be materialized regardless. However, if the complexity model is extended for finer-grained ranking beyond threshold checks, this could undercount actual expression cost.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/geo/GeoDistanceMetersFunctionFactory.java`
around lines 194 - 197, The getComplexity() overrides in
ConstPoint1GeoDistanceFunction and GeoDistanceFunction return a flat
COMPLEXITY_GEO and do not include their child functions' complexity; update each
getComplexity() to aggregate child complexities by calling
Function.addComplexity(...) (or summing child.getComplexity()) for their managed
child functions (e.g., lat2Func, lon2Func, latFunc, lonFunc, pointFunc) and
return COMPLEXITY_GEO plus that aggregated value so the overall expression cost
includes children.

core/src/main/java/io/questdb/griffin/engine/functions/math/RadiansDoubleFunctionFactory.java (1)

33-33: Stray blank line left by removed UnaryFunction import.

The blank line at line 33 is a formatting artifact; removing it keeps the import block tidy.

🧹 Suggested cleanup

 import io.questdb.griffin.engine.functions.DoubleFunction;
-
 import io.questdb.griffin.engine.functions.constants.DoubleConstant;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/math/RadiansDoubleFunctionFactory.java`
at line 33, Remove the stray blank line in the import block of
RadiansDoubleFunctionFactory.java left after removing the UnaryFunction import;
open the import section in RadiansDoubleFunctionFactory and delete the empty
line so imports are contiguous and properly formatted (look for the class
RadiansDoubleFunctionFactory and the surrounding import declarations to locate
the spot).

core/src/test/java/io/questdb/test/griffin/GroupByTest.java (1)

2738-2785: Same applies here — plan expectation correctly updated.

testLiftAliasesFromInnerSelect9 and testGroupByWithAliasClash6 appear to test an identical query (same DDL, same SQL, same expected rows, same expected plan). If this duplication is pre-existing it's fine to leave it, but it may be an opportunity to consolidate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/test/java/io/questdb/test/griffin/GroupByTest.java` around lines
2738 - 2785, Duplicate test detected: testLiftAliasesFromInnerSelect9 appears
identical to testGroupByWithAliasClash6 (same DDL, SQL, expected rows and plan).
Either remove one of the tests or consolidate them by extracting the shared
setup and assertions into a single helper method (e.g.,
createTestForAliasClash(query, expectedPlan, expectedRows)) and call it from
both test names if both must remain; update or remove the redundant test
(testLiftAliasesFromInnerSelect9 or testGroupByWithAliasClash6) and ensure the
remaining test keeps the same execute/create table calls, assertPlanNoLeakCheck,
and assertQueryNoLeakCheck expectations.

core/src/main/java/io/questdb/griffin/engine/functions/math/SubIntFunctionFactory.java (1)

49-97: Add isOperator() override to SubtractIntVVFunc.

SubtractIntVVFunc should override isOperator() to return true, consistent with other arithmetic binary operators. For example, MulIntFunctionFactory's Func class explicitly overrides isOperator() to return true. The default implementation in Function returns false, causing the subtraction operator to be misclassified as a non-operator, which is inconsistent with similar operations and may impact toPlan() behavior and future logic that depends on this flag.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/functions/math/SubIntFunctionFactory.java`
around lines 49 - 97, SubtractIntVVFunc is missing an override of isOperator()
so it inherits the default false; add a public boolean isOperator() override in
SubtractIntVVFunc that returns true to classify this binary subtraction function
as an operator (mirror how MulIntFunctionFactory's Func implements
isOperator()), ensuring to place the method within the SubtractIntVVFunc class
alongside other overrides.

core/src/test/java/io/questdb/test/griffin/engine/orderby/OrderBySortKeyMaterializationTest.java (2)

1629-1658: Clarify the materialization exclusion criteria for IPv4.

The comment says "IPv4 is excluded from materialization (no arithmetic on this type)" but IPv4 is a fixed-size 4-byte type. The actual exclusion reason appears to be a design decision in the materializer (perhaps only numeric/temporal/boolean/geo/decimal types are routed), not a size constraint. Meanwhile, UUID exclusion at Line 1772 is explained as "> Long.BYTES" which is a different criterion. Consider aligning the comments to reference the actual filtering logic in SortKeyMaterializingRecordCursor/MaterializedRecord for clarity.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/orderby/OrderBySortKeyMaterializationTest.java`
around lines 1629 - 1658, Update the inline comment in
OrderBySortKeyMaterializationTest to accurately reflect the materializer's
filtering logic: replace "IPv4 is excluded from materialization (no arithmetic
on this type)" with a brief note that IPv4 is excluded because the materializer
routes only certain categories (numeric/temporal/boolean/geo/decimal) for
materialization per the logic in
SortKeyMaterializingRecordCursor/MaterializedRecord, and similarly adjust the
UUID comment to reference the size-based rule (> Long.BYTES) used by the same
materializer code so both comments consistently point to the actual filtering
criteria in SortKeyMaterializingRecordCursor/MaterializedRecord.

---

1748-1758: Different assertQueryNoLeakCheck overload used only here.

This test uses the 5-argument overload assertQueryNoLeakCheck(expected, query, "x", true, true) while all other tests use the simpler 2-argument form. If the extra parameters (expected timestamp column, supportsRandomAccess flags) are needed specifically for TIMESTAMP sort-key verification, a brief inline comment explaining why would help future maintainers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/test/java/io/questdb/test/griffin/engine/orderby/OrderBySortKeyMaterializationTest.java`
around lines 1748 - 1758, The test call to assertQueryNoLeakCheck is the only
one using the 5-argument overload (assertQueryNoLeakCheck(expected, query, "x",
true, true)); either revert it to the standard 2-argument form
assertQueryNoLeakCheck(expected, query) if the extra parameters are not
required, or keep the 5-arg call but add a one-line inline comment immediately
above it explaining that the third and two boolean flags are intentionally
supplied to verify TIMESTAMP sort-key behavior (i.e., specify expected timestamp
column "x" and supportsRandomAccess=true flags) so future maintainers understand
why this test differs from others.

core/src/main/java/io/questdb/griffin/engine/orderby/SortKeyMaterializingRecordCursor.java (1)

125-144: Missing guard for the -1L no-entry sentinel returned by rowIdToOrdinal.

rowIdToOrdinal was constructed with noEntryValue = -1L. When materialized = true, rowIdToOrdinal.get(rowId) silently returns -1L for any rowId not in the map — including the edge case where baseRecord.getRowId() equals Long.MIN_VALUE (the noEntryKey, which put silently ignores). Passing ordinal -1L to recordA.setOrdinal() causes MaterializedRecord to compute a negative buffer offset and read native memory before the buffer's base address.

The same pattern repeats at line 178 inside recordAt().

While in the normal two-pass flow this should never fire, the invariant is invisible to the JVM and failures would manifest as silent data corruption or a native crash. An assertion or explicit check at both sites would make the invariant explicit and the failure mode detectable:

♻️ Proposed defensive guard

         if (materialized) {
             ordinal = rowIdToOrdinal.get(rowId);
+            assert ordinal != -1L : "rowId not found in materialization map: " + rowId;
         } else {

         baseCursor.recordAt(((MaterializedRecord) record).getBaseRecord(), atRowId);
         final long ordinal = rowIdToOrdinal.get(atRowId);
+        assert ordinal != -1L : "rowId not found in materialization map: " + atRowId;
         ((MaterializedRecord) record).setOrdinal(ordinal);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@core/src/main/java/io/questdb/griffin/engine/orderby/SortKeyMaterializingRecordCursor.java`
around lines 125 - 144, In SortKeyMaterializingRecordCursor, guard against the
rowIdToOrdinal no-entry sentinel (-1L) returned by rowIdToOrdinal.get(rowId)
before passing the value into recordA.setOrdinal: in hasNext() (where
materialized is true) check if ordinal == -1L and throw an IllegalStateException
(or assert) with a clear message referencing rowId and that materialization
invariant was violated; apply the same defensive check in recordAt() where
rowIdToOrdinal.get(...) is used, and ensure any code paths that compute/put
ordinals (nextOrdinal/rowIdToOrdinal.put(...) and appendValue(...)) remain
unchanged — this makes the invariant explicit and fails fast instead of
computing negative offsets in MaterializedRecord.