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 pls critically review this change and also see if claims are valid (including issue PR claims to have fixed)

#!/bin/bash

# Get the PR diff to see the actual changes
gh pr view 6733 --json files,additions,deletions,changedFiles,body,number,title | jq '.'

# Get the issue `#6494` details to verify the claim
gh issue view 6494 --json number,title,body,labels,state | jq '.'

# Get the actual diff
gh pr diff 6733

#!/bin/bash

# Verify isPartitionsKeyword() method exists
echo "=== Checking if isPartitionsKeyword() method exists ==="
rg -n "isPartitionsKeyword" --type=java -C 3

# Check for other keyword checking methods to understand the pattern
echo -e "\n=== Pattern of keyword checking methods ==="
rg -n "boolean isPartition.*Keyword" --type=java -A 2

# Look for other ALTER TABLE CONVERT test scenarios
echo -e "\n=== Other ALTER TABLE CONVERT test methods ==="
rg -n "testConvert" core/src/test/java/io/questdb/test/griffin/AlterTableConvertPartitionTest.java

# Check for other usages of PARTITION keyword in ALTER TABLE context
echo -e "\n=== Other PARTITION keyword usages in ALTER TABLE ==="
rg -n "isPartitionKeyword" core/src/main/java/io/questdb/griffin/SqlCompilerImpl.java -C 5

# Check if there are LIST or specific partition convert scenarios
echo -e "\n=== Looking for CONVERT PARTITION LIST syntax ==="
rg -n "convert.*partition.*list" --type=java -i -C 2

@bluestreak01, I've completed the critical review. The claims are valid (issue #6494 exists and matches), but I have significant concerns about this implementation:

❌ Critical Issues

1. Grammar Inconsistency Across ALTER TABLE Operations

The implementation creates an arbitrary inconsistency:

// CONVERT - now accepts BOTH (after this PR)
if (!isPartitionKeyword(tok) && !isPartitionsKeyword(tok))

// ATTACH - accepts ONLY singular (line 2124-2130)
if (isPartitionKeyword(tok))

// DETACH - accepts ONLY singular (line 2132-2138) 
if (isPartitionKeyword(tok))

// DROP - accepts ONLY singular (line 2110)
else if (isPartitionKeyword(tok))

// SQUASH - accepts ONLY plural (line 2313)
if (isPartitionsKeyword(tok))

Why does CONVERT get special treatment? Other operations consistently enforce singular/plural. What's the design principle here?

2. Inadequate Test Coverage

The new test only covers the WHERE clause:

"alter table " + tableName + " convert " + partitionKeyword + " to parquet where timestamp > 0"

But completely ignores the LIST syntax, which is heavily used:

• Line 115: convert partition to parquet list '2024-06'
• Line 119: convert partition to parquet list '1970-01'
• Line 138: convert partition to parquet list '2024-06-10', '2024-06-11'

Where are the tests for:

ALTER TABLE x CONVERT PARTITIONS TO PARQUET LIST '2024-06-10'
ALTER TABLE x CONVERT PARTITIONS TO NATIVE LIST '2024-06-10', '2024-06-11'

3. Semantic Ambiguity

Now this is valid:

-- Plural form for single partition - semantically wrong?
ALTER TABLE x CONVERT PARTITIONS TO PARQUET LIST '2024-06-10'

Should plural really work when converting a single partition? Other DBs (PostgreSQL, MySQL) are strict about this for good reason - it guides users toward correct SQL.

4. Missing Design Rationale

The issue shows: convert partitions to parquet where timestamp < '2025-11-28'

This converts multiple partitions, so plural makes semantic sense. But:

• Why allow plural for LIST '2024-06-10' (single partition)?
• Why not make ATTACH/DETACH/DROP consistent too?
• What about SQUASH, which only accepts plural?

🤔 Questions for the Author

1. Why not align ALL partition operations to accept both forms if this is about UX?
2. Or conversely, why not enforce semantic correctness (singular for one, plural for many)?
3. Was there consideration for parser ambiguity or future grammar conflicts?
4. Should the error message guide users on when to use which form?

✅ What's Correct

• Issue #6494 exists and is accurately described
• isPartitionsKeyword() method exists
• Basic implementation works
• Error message updated

📋 Recommendations

1. Add comprehensive tests for LIST syntax with plural keyword
2. Document the design decision - why CONVERT is special
3. Consider grammar-wide consistency - either make all operations flexible or enforce semantics
4. Add grammar documentation noting both forms are accepted

This feels like a quick fix without considering the broader grammar design. The friction is real, but the solution creates technical debt.

Learnt from: jerrinot
Repo: questdb/questdb PR: 0
File: :0-0
Timestamp: 2026-02-04T11:18:36.475Z
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.

Thanks for the review.

Regarding the scope:

• The original issue CONVERT PARTITION should accept plural PARTITIONS as well #6494 specifically requests plural support for CONVERT only, so I kept the scope focused. Happy to extend to other operations (ATTACH/DETACH/DROP) if you think consistency is preferred.

Regarding test coverage:

• I'll add tests for List syntax:
  • CONVERT PARTITIONS TO PARQUET LIST '...'
  • CONVERT PARTITIONS TO NATIVE LIST '...', '...'

I'll push the additional tests shortly. Thanks!