fix(sql): fix ADD COLUMN IF NOT EXISTS for DECIMAL, GEOHASH, and array types#6753
fix(sql): fix ADD COLUMN IF NOT EXISTS for DECIMAL, GEOHASH, and array types#6753bluestreak01 merged 7 commits intomasterfrom
Conversation
… EXISTS In `alterTableAddColumn()`, the IF NOT EXISTS path called `ColumnType.typeOf(tok)` which returns the base DECIMAL (34) or GEOHASH (23) constant. But `tableMetadata.getColumnType()` returns the fully encoded type (with precision/scale/storage-size for DECIMAL, or bit precision for GEOHASH). The comparison always failed because the base constant never equals an encoded type. The fix resolves parameterized types before comparing: - DECIMAL: call `SqlParser.parseDecimalColumnType(lexer)` - GEOHASH: call new `parseGeoHashColumnType(lexer)` helper Also extracted the inline GEOHASH parsing in `addColumnWithType` into the reusable `parseGeoHashColumnType` static method, and captured the type keyword position so error messages point at the type name rather than the closing parenthesis. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
mtopolnik
changed the title
mtopolnik
changed the title
mtopolnik
added
Bug
mtopolnik
changed the title
…nsistency - Move parseGeoHashColumnType from SqlCompilerImpl to SqlParser as a public static method, co-locating it with parseDecimalColumnType for consistency - Replace ColumnType.typeOf() with SqlUtil.toPersistedType() in the IF NOT EXISTS path to match the addColumnWithType path and give more specific error messages for non-persisted types - Add array dimensionality parsing to the IF NOT EXISTS path so that DOUBLE[] columns are correctly recognized and compared - Add tests for array type handling in ADD COLUMN IF NOT EXISTS Co-Authored-By: Claude Opus 4.6 <[email protected]>
mtopolnik
force-pushed
the
mt_fix-add-column-if-exists
branch
from
7754ddc to
3e9a9c5
Compare
Add two missing checks from the normal ADD COLUMN flow to the IF NOT EXISTS path: 1. Unsupported array element type validation — e.g. `INT[]` now correctly reports "unsupported array element type" instead of falling through to a misleading "column already exists with different type" error. 2. Unmatched `]` bracket detection — a stray `]` after the type name (e.g. `double]`) is now caught with a helpful error message pointing at the bracket position. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…STS path Move GEOHASH type parsing before the unmatched `]` bracket check in the normal addColumnWithType() path, making it consistent with the IF NOT EXISTS path. Previously GEOHASH was parsed after the bracket check while DECIMAL was parsed before it, which could produce different error messages for malformed input between the two code paths. Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
PR Review by Claude
Overall: Looks good. The fix is well-scoped and the approach is sound — the IF NOT EXISTS path now mirrors the normal addColumnWithType path for type resolution, ensuring the comparison uses fully-encoded types rather than bare base types.
Positives
-
Good deduplication of GEOHASH parsing — moving
parseGeoHashColumnTypetoSqlParser(next toparseDecimalColumnType) and calling it from both paths eliminates the inline copy inaddColumnWithType. The new method is a clean 1:1 extraction of the old inline code. -
Consistent use of
SqlUtil.toPersistedTypeinstead of rawColumnType.typeOfin the IF NOT EXISTS path, giving better error messages for non-persisted types like CURSOR. -
Good test coverage — tests for DECIMAL (with and without explicit precision), GEOHASH, arrays, unsupported array element types, and the unmatched bracket case.
-
Error positions are correct —
typePositionis captured right after reading the type token and used for all error messages in the IF NOT EXISTS block.
Issues
1. IF NOT EXISTS path doesn't consume remaining tokens on success (medium)
When the column exists and types match, the code hits break at line 968. But if the statement had additional clauses after the type (e.g., ALTER TABLE x ADD COLUMN IF NOT EXISTS col SYMBOL CAPACITY 1024 CACHE, other_col INT), those tokens are left unconsumed. The normal addColumnWithType path parses SYMBOL capacity/cache/index options. The IF NOT EXISTS path ignores them, which means:
ALTER TABLE x ADD COLUMN IF NOT EXISTS col SYMBOL CAPACITY 1024 CACHE— theCAPACITY 1024 CACHEtokens are silently dropped when the column already exists- If there are more columns after a comma, they'd also be silently dropped
This was pre-existing behavior (not introduced by this PR), but it's worth noting as a follow-up item since you're already touching this code.
2. Minor: bare DECIMAL in the IF NOT EXISTS path (low)
The test testAddDuplicateColumnIfNotExistsDecimalDefault adds a column as bare DECIMAL then checks it with bare DECIMAL again. This works because SqlParser.parseDecimalColumnType defaults to (18,3) in both cases. Good — but there's no test for the cross-case: adding as bare DECIMAL and checking with explicit DECIMAL(18, 3) (or vice versa). Adding that would further validate the equivalence. Not blocking.
3. No test for GEOHASH missing precision in IF NOT EXISTS path (low)
There's no test for ALTER TABLE x ADD COLUMN IF NOT EXISTS geo_col GEOHASH (missing precision). The parsing would throw "missing GEOHASH precision" from the new parseGeoHashColumnType, but there's no explicit test asserting this error in the IF NOT EXISTS context. Not blocking.
Summary
The core logic is correct. The two code paths (normal add and IF NOT EXISTS) now produce the same fully-encoded type for DECIMAL, GEOHASH, and array types before comparison. Approve, with optional suggestions to add the cross-case DECIMAL test and to note the pre-existing SYMBOL options consumption gap as a follow-up.
Add a new "QuestDB's SQL dialect" section documenting key SQL dialect differences: multidimensional array support with dimensionality encoded in the column type, and the absence of DELETE support. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…cation The ADD COLUMN IF NOT EXISTS path duplicated ~30 lines of type-parsing logic from addColumnWithType. This duplication was the root cause of the bugs this branch already fixes (missing DECIMAL/GEOHASH/array handling). The duplicated code also broke without consuming trailing column options (SYMBOL CAPACITY, CACHE, INDEX, NOT NULL), leaving tokens in the lexer. Changes: - Make addColumnWithType accept nullable AlterOperationBuilder so the IF NOT EXISTS path can call it with null to parse and validate the full column definition without actually adding the column. - Replace the duplicated type-parsing block in the IF NOT EXISTS path with a call to addColumnWithType(null, ...), which properly consumes trailing SYMBOL options and NOT NULL tokens. - After addColumnWithType returns, handle delimiters (`,`, `;`, EOF) the same way the normal path does, including support for multi-column IF NOT EXISTS statements. - Fix the unmatched bracket error position to use typePosition consistently (was using lexer.lastTokenPosition() for `]` in the IF NOT EXISTS path but typePosition in the normal path). - Add tests for: IF NOT EXISTS with SYMBOL options, multi-column IF NOT EXISTS, and trailing garbage rejection. Co-Authored-By: Claude Opus 4.6 <[email protected]>
[PR Coverage check]😍 pass : 36 / 37 (97.30%) file detail
|
3 participants
Fixes #6750
Summary
The
ALTER TABLE t ADD COLUMN IF NOT EXISTS col <type>path did notfully resolve parameterized or compound column types before comparing
with existing metadata, causing false type-mismatch errors.
Bug fixes
ColumnType.typeOf()whichreturns the base type constant, but metadata stores the fully encoded
type (with precision/scale/bits). Fixed by parsing DECIMAL and GEOHASH
parameters before comparing.
DOUBLE[]): the IF NOT EXISTS path did not parse arraydimensionality brackets, causing type mismatch errors and unconsumed tokens.
Fixed by calling
SqlUtil.parseArrayDimensionality()before comparing.INT[]) in the IF NOT EXISTS pathnow correctly report "unsupported array element type" instead of falling
through to a misleading type-mismatch error.
]bracket after a type name (e.g.double]) is now detectedin the IF NOT EXISTS path with a helpful error pointing at the bracket.
Consistency improvements
parseGeoHashColumnTypefromSqlCompilerImpl(private) toSqlParser(public) to co-locate it withparseDecimalColumnTypeColumnType.typeOf()withSqlUtil.toPersistedType()in theIF NOT EXISTS path for consistency with
addColumnWithType, givingmore specific error messages for non-persisted types like CURSOR
Test plan
ADD COLUMN IF NOT EXISTS col DECIMAL(48, 18)is a no-op when column already exists with same typeADD COLUMN IF NOT EXISTS col DECIMAL(bare, defaults to 18,3) is a no-op when column exists as DECIMAL(18,3)ADD COLUMN IF NOT EXISTS col DECIMAL(18, 3)correctly reports type mismatch when column exists as DECIMAL(48, 18)ADD COLUMN IF NOT EXISTS col GEOHASH(5c)is a no-op when column already exists with same typeADD COLUMN IF NOT EXISTS col GEOHASH(3c)correctly reports type mismatch when column exists as GEOHASH(5c)ADD COLUMN IF NOT EXISTS col DOUBLE[]is a no-op when column already exists with same typeADD COLUMN IF NOT EXISTS col DOUBLE[][]correctly reports type mismatch when column exists as DOUBLE[]ADD COLUMN IF NOT EXISTS col INT[]reports "unsupported array element type" in the IF NOT EXISTS pathADD COLUMN IF NOT EXISTS col double]detects unmatched bracket with correct error position🤖 Generated with Claude Code