Copilot commented on code in PR #13037:
URL: https://github.com/apache/ignite/pull/13037#discussion_r3091398566
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -177,6 +183,37 @@ private void handle0(CreateTableCommand cmd) throws
IgniteCheckedException {
}
}
+ /** */
+ private void checkKVWrappedParam(CreateTableCommand cmd) {
+ boolean wrapKey = cmd.wrapKey();
+
+ if (!wrapKey) {
+ if (cmd.primaryKeyColumns().size() > 1) {
+ throw new IgniteSQLException(WRAP_KEY + " parameter cannot be
\"false\" when composite primary key exists.",
+ IgniteQueryErrorCode.PARSING);
+ }
+
+ if (!F.isEmpty(cmd.keyTypeName())) {
+ throw new IgniteSQLException(WRAP_KEY + " cannot be \"false\"
when " + KEY_TYPE + " is defined.",
+ IgniteQueryErrorCode.PARSING);
+ }
+ }
+
+ boolean wrapVal = cmd.wrapValue();
+
+ if (!wrapVal) {
+ if (cmd.columns().size() - cmd.primaryKeyColumns().size() > 1) {
+ throw new IgniteSQLException(WRAP_VALUE + " parameter cannot
be \"false\" with multiple columns.",
+ IgniteQueryErrorCode.PARSING);
+ }
+
+ if (!F.isEmpty(cmd.valueTypeName())) {
+ throw new IgniteSQLException(WRAP_VALUE + " cannot be
\"false\" when " + VALUE_TYPE + " is defined.",
+ IgniteQueryErrorCode.PARSING);
+ }
+ }
Review Comment:
`wrap_key` is stored as a primitive boolean with default `false`, so when
the option is omitted `cmd.wrapKey()` is `false`. With the newly added
validation this will now reject any table that has a composite primary key (and
also makes it impossible to distinguish “wrap_key not specified” from
“wrap_key=false”). Consider switching `CreateTableCommand.wrapKey` to a
tri-state (e.g., `@Nullable Boolean`) or adding a “specified” flag, then
compute the effective wrap behavior (similar to H2) and only throw when the
user explicitly set `wrap_key=false`. Also, `cmd.wrapKey()` is currently only
used for this validation and never affects `toQueryEntity`, so `WITH
wrap_key=true` for a single-column PK won’t change key wrapping unless
`key_type` is also provided.
```suggestion
int pkColsCnt = cmd.primaryKeyColumns().size();
int valColsCnt = cmd.columns().size() - pkColsCnt;
boolean wrapKey = cmd.wrapKey() || pkColsCnt > 1 ||
!F.isEmpty(cmd.keyTypeName());
boolean wrapVal = cmd.wrapValue() || valColsCnt > 1 ||
!F.isEmpty(cmd.valueTypeName());
/*
* CreateTableCommand stores wrap_key / wrap_value as primitive
booleans, so "option omitted"
* is indistinguishable here from an explicit "... = false". Because
of that, this handler
* must not reject composite PKs or multi-column values based on the
raw boolean alone.
*
* Compute effective wrapping instead, matching the table shape and
explicit type options:
* - composite PK or KEY_TYPE implies wrapped key;
* - multiple value columns or VALUE_TYPE implies wrapped value.
*
* Explicit rejection of "... = false" requires a tri-state flag in
CreateTableCommand and
* cannot be implemented safely in this file alone.
*/
if (!wrapKey && !wrapVal) {
// No-op: both key and value remain unwrapped only when their
effective shape allows it.
}
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -386,6 +423,22 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) &&
cmd.primaryKeyColumns().size() =
res = new QueryEntityEx(res).implicitPk(true);
}
+ if (!cmd.wrapValue()) {
+ ColumnDefinition valCol = null;
+
+ for (ColumnDefinition col : cmd.columns()) {
+ if (!cmd.primaryKeyColumns().contains(col.name())) {
+ valCol = col;
+ break;
+ }
+ }
+
+ assert valCol != null;
+
+ valTypeName =
Commons.typeFactory().getResultClass(valCol.type()).getTypeName();
+ res.setValueFieldName(valCol.name());
+ }
Review Comment:
When `wrap_value=false`, this block assumes there is exactly one non-PK
column and uses `assert valCol != null`. However, `checkKVWrappedParam` only
rejects the case with more than one non-PK column and allows the case with zero
value columns (e.g., `CREATE TABLE t(id INT PRIMARY KEY) WITH
"wrap_value=false"`), which would trigger this assertion at runtime. Please add
explicit validation and throw a parsing `IgniteSQLException` with a clear
message when there is no value column to unwrap (or define the intended
behavior for key-only tables).
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -214,7 +251,7 @@ private void handle0(AlterTableAddCommand cmd) throws
IgniteCheckedException {
if (QueryUtils.isSqlType(typeDesc.valueClass())) {
throw new SchemaOperationException("Cannot add column(s)
because table was created " +
- "with WRAP_VALUE=false option.");
+ "based on cache configured with built-in types or with " +
WRAP_VALUE + "=false option.");
}
Review Comment:
This new error message no longer contains the previous substring "table was
created with WRAP_VALUE=false option" (there are extra words between "created"
and "with"). Existing tests in
`TableDdlIntegrationTest#doTestAlterTableOnFlatValue` still assert the old
wording and will fail; either update the test expectation to match the new
message or keep the old phrasing for compatibility.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -88,6 +91,20 @@ public class DdlSqlToCommandConverter {
return ((SqlIdentifier)opt.value()).getSimple();
};
+ /** Processor that validates that value can be parsed as boolean. */
+ private static final BiFunction<IgniteSqlCreateTableOption,
PlanningContext, Boolean> VALUE_IS_BOOL_IDENTIFIER_VALIDATOR =
+ (opt, ctx) -> {
+ SqlNode val = opt.value();
+ if (!(val instanceof SqlIdentifier) ||
!((SqlIdentifier)val).isSimple())
+ throwOptionParsingException(opt, "a simple identifier",
ctx.query());
+
+ String simple =
((SqlIdentifier)val).getSimple().toLowerCase(Locale.ROOT);
+ if (!"true".equals(simple) && !"false".equals(simple))
+ throwOptionParsingException(opt, "Unexpected identifier: " +
simple, ctx.query());
+
+ return Boolean.valueOf(((SqlIdentifier)val).getSimple());
+ };
+
Review Comment:
`VALUE_IS_BOOL_IDENTIFIER_VALIDATOR` only accepts `SqlIdentifier` values, so
`WITH wrap_key=true` / `WITH wrap_value=false` written as unquoted boolean
literals will be rejected (other options like `ENCRYPTED` already accept
`SqlLiteral` BOOLEAN). Consider accepting `SqlLiteral` of BOOLEAN type in
addition to identifiers for consistency. Also, the
`throwOptionParsingException` call uses `"Unexpected identifier: ..."` as the
*expected* string, which produces a confusing error message; it should say
something like "expected true or false".
```suggestion
if (val instanceof SqlLiteral && ((SqlLiteral)val).getTypeName()
== BOOLEAN)
return ((SqlLiteral)val).booleanValue();
if (val instanceof SqlIdentifier &&
((SqlIdentifier)val).isSimple()) {
String simple =
((SqlIdentifier)val).getSimple().toLowerCase(Locale.ROOT);
if ("true".equals(simple) || "false".equals(simple))
return Boolean.valueOf(simple);
}
throwOptionParsingException(opt, "true or false", ctx.query());
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
