• 🔍 Trigger a full review

Walkthrough

The PR introduces a new PercentRankFunctionFactory implementing the percent_rank() window function with three execution modes (with partition and order, with order only, with neither), removes deprecated window function factories from the registry, and adds comprehensive test coverage for the new function.

Changes

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• fix(sql): fix intermittent failures with SQL window functions #6030: Refactors AbstractWindowFunctionFactory and WindowContext validation machinery that the new PercentRankFunctionFactory relies upon for constraint enforcement.

Suggested labels

New feature, SQL

Suggested reviewers

• puzpuzpuz

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.

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

@CodeRabbit can you review this window function implementation pls?

#!/bin/bash
# Read the main implementation file
cat core/src/main/java/io/questdb/griffin/engine/functions/window/PercentRankFunctionFactory.java

#!/bin/bash
# Find and read test files for percent_rank
fd -e java -x grep -l "percent_rank" {} \; | grep -i test | head -5 | xargs cat

#!/bin/bash
# Check if there are any other related files or similar window function implementations for comparison
fd "RankFunctionFactory.java" --type f

#!/bin/bash
# Read RankFunctionFactory for comparison
cat core/src/main/java/io/questdb/griffin/engine/functions/window/RankFunctionFactory.java

@bluestreak01 Excellent implementation! I've thoroughly reviewed the percent_rank() window function. Here's my analysis:

✅ Strengths

1. Correct SQL Standard Implementation: The formula (rank - 1) / (total_rows - 1) is properly implemented with correct edge case handling (returns 0 for single row).

2. Well-Structured Design: The three-class approach mirrors RankFunctionFactory perfectly:

   • PercentRankNoOrderFunction for no ORDER BY (all peers → percent_rank = 0)
   • PercentRankFunction for ORDER BY without PARTITION BY
   • PercentRankOverPartitionFunction for both PARTITION BY and ORDER BY

3. Correct TWO_PASS Strategy: Unlike rank() which can use ZERO_PASS, percent_rank() correctly uses TWO_PASS because it needs the total row count. The approach of:

   • Pass1: Store rank temporarily as LONG
   • Pass2: Compute and store percent_rank as DOUBLE using total count

   is elegant and efficient.

4. Count Tracking Logic: The count management is correct throughout:

   // Pass1: stores count + 1
   mapValue.putLong(2, count + 1);
   
   // Pass2: retrieves and adjusts
   long totalRows = mapValue.getLong(2) - 1;  // Gets actual row count

5. Comprehensive Test Coverage (93.67%): Your tests are exemplary:

   • Edge cases: single row, empty table, all ties, single-row partitions
   • Multiple ORDER BY columns, DESC order
   • NULLs in ORDER BY and PARTITION BY
   • Combined with other ranking functions
   • Error cases (no OVER, RESPECT/IGNORE NULLS, framing)
   • EXPLAIN plans
   • Large datasets (1000 rows)

6. Proper Resource Management: close(), reset(), reopen(), and toTop() are all correctly implemented.

📝 Minor Observation

The toPlan() implementations don't include the ORDER BY clause details:

• PercentRankFunction.toPlan(): shows "() over ()" but doesn't mention ORDER BY
• PercentRankOverPartitionFunction.toPlan(): shows partition by but not ORDER BY

However, this is consistent with RankFunctionFactory and doesn't affect functionality since the EXPLAIN output gets the full context from the window framework. Your test testPercentRankExplainPlan() confirms the output is correct.

🎯 Verdict

This is production-ready code. The implementation is clean, efficient, follows established patterns, and has excellent test coverage. The logic for handling partitions, peer groups, and edge cases is all correct. Great work! 🚀

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

@CodeRabbit i added "order by" if exists to the plan. Please review.

#!/bin/bash
# Read the updated PercentRankFunctionFactory to see the changes
cat core/src/main/java/io/questdb/griffin/engine/functions/window/PercentRankFunctionFactory.java

@bluestreak01 Excellent update! 👍 I've reviewed the changes to the toPlan() methods.

✅ Changes Look Perfect

The ORDER BY clause is now properly included in the plan output:

1. PercentRankFunction.toPlan() (Line 231-237):

   sink.val(" over (");
   sink.val("order by ");
   sink.val(orderBy);
   sink.val(')');

   Now shows percent_rank() over (order by ...) instead of just percent_rank() over ()

2. PercentRankOverPartitionFunction.toPlan() (Line 391-399):

   sink.val(" over (");
   sink.val("partition by ");
   sink.val(partitionByRecord.getFunctions());
   sink.val(" order by ");
   sink.val(orderBy);
   sink.val(')');

   Now shows the complete specification: percent_rank() over (partition by ... order by ...)

3. PercentRankNoOrderFunction.toPlan() correctly omits ORDER BY since it handles the no-order case.

The orderBy field is properly captured during initRecordComparator() and used in the plan output. This makes the EXPLAIN plans much more informative and complete! The implementation is spot-on. 🚀

Learnt from: bluestreak01
Repo: questdb/questdb PR: 6643
File: core/src/main/java/io/questdb/griffin/SqlOptimiser.java:8671-8687
Timestamp: 2026-01-20T00:59:19.630Z
Learning: QuestDB’s parser rejects pure window functions (e.g., row_number(), rank()) used without an OVER clause; such invocations are invalid and fail early, so validators don’t need to catch pure-name window functions in window specs.

[PR Coverage check]

😍 pass : 156 / 166 (93.98%)

file detail