korlov42 commented on code in PR #783:
URL: https://github.com/apache/ignite-3/pull/783#discussion_r854326113
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -388,104 +428,170 @@ private void ensureSchemaExists(PlanningContext ctx,
String schemaName) {
}
/**
- * Short cut for validating that option value is a simple identifier.
+ * Throws exception with message relates to validation of create table
option.
*
- * @param opt An option to validate.
- * @param ctx Planning context.
+ * @param opt An option which validation was failed.
+ * @param exp A string representing expected values.
+ * @param qry A query the validation was failed for.
*/
- private String paramIsSqlIdentifierValidator(IgniteSqlCreateTableOption
opt, PlanningContext ctx) {
- if (!(opt.value() instanceof SqlIdentifier) || !((SqlIdentifier)
opt.value()).isSimple()) {
- throwOptionParsingException(opt, "a simple identifier",
ctx.query());
+ private static void throwOptionParsingException(IgniteSqlCreateTableOption
opt, String exp, String qry) {
Review Comment:
looks like this is not used anymore
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -47,18 +49,34 @@
* An implementation of the {@link PrepareService} that uses a Calcite-based
query planner to validate and optimize a given query.
*/
public class PrepareServiceImpl implements PrepareService {
- private final DdlSqlToCommandConverter ddlConverter = new
DdlSqlToCommandConverter();
+ private final DdlSqlToCommandConverter ddlConverter;
+
+ /**
+ * Constructor.
+ *
+ * @param dataStorageManager Data storage manager.
+ * @param tablesConfig Tables configuration.
+ * @param dataStorageFields Data storage fields. Mapping: Data storage
name -> field name -> field type.
+ */
+ public PrepareServiceImpl(
+ DataStorageManager dataStorageManager,
+ TablesConfiguration tablesConfig,
+ Map<String, Map<String, Class<?>>> dataStorageFields
+ ) {
+ ddlConverter = new DdlSqlToCommandConverter(
+ dataStorageFields,
+ () ->
dataStorageManager.defaultDataStorage(tablesConfig.defaultDataStorage().value())
Review Comment:
IMO, the `default` method should not take any parameters.
`TablesConfiguration` is kinda internal thing of
TableManager/DataStorageManager. It's better to keep it out from other managers
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -134,12 +163,23 @@ private CreateTableCommand
convertCreateTable(IgniteSqlCreateTable createTblNode
createTblCmd.schemaName(deriveSchemaName(createTblNode.name(), ctx));
createTblCmd.tableName(deriveObjectName(createTblNode.name(), ctx,
"tableName"));
createTblCmd.ifTableExists(createTblNode.ifNotExists());
+ createTblCmd.dataStorage(driveDataStorage(createTblNode.engineName(),
ctx));
if (createTblNode.createOptionList() != null) {
- for (SqlNode optNode : createTblNode.createOptionList().getList())
{
- IgniteSqlCreateTableOption opt = (IgniteSqlCreateTableOption)
optNode;
+ for (SqlNode optionNode :
createTblNode.createOptionList().getList()) {
+ IgniteSqlCreateTableOption option =
(IgniteSqlCreateTableOption) optionNode;
+
+ assert option.key().isSimple() : option.key();
+
+ String optionKey = option.key().getSimple();
- tblOptionProcessors.getOrDefault(opt.key(),
UNSUPPORTED_OPTION_PROCESSOR).process(opt, ctx, createTblCmd);
+ if (tableOptionInfos.containsKey(optionKey)) {
+ processTableOption(tableOptionInfos.get(optionKey),
option, ctx, createTblCmd);
+ } else if
(dataStorageOptionInfos.get(createTblCmd.dataStorage()).containsKey(optionKey))
{
Review Comment:
Does it make sense to require an engine to be specified explicitly if any
storage's option is provided?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -134,12 +163,23 @@ private CreateTableCommand
convertCreateTable(IgniteSqlCreateTable createTblNode
createTblCmd.schemaName(deriveSchemaName(createTblNode.name(), ctx));
createTblCmd.tableName(deriveObjectName(createTblNode.name(), ctx,
"tableName"));
createTblCmd.ifTableExists(createTblNode.ifNotExists());
+ createTblCmd.dataStorage(driveDataStorage(createTblNode.engineName(),
ctx));
if (createTblNode.createOptionList() != null) {
- for (SqlNode optNode : createTblNode.createOptionList().getList())
{
- IgniteSqlCreateTableOption opt = (IgniteSqlCreateTableOption)
optNode;
+ for (SqlNode optionNode :
createTblNode.createOptionList().getList()) {
+ IgniteSqlCreateTableOption option =
(IgniteSqlCreateTableOption) optionNode;
+
+ assert option.key().isSimple() : option.key();
+
+ String optionKey = option.key().getSimple();
- tblOptionProcessors.getOrDefault(opt.key(),
UNSUPPORTED_OPTION_PROCESSOR).process(opt, ctx, createTblCmd);
+ if (tableOptionInfos.containsKey(optionKey)) {
+ processTableOption(tableOptionInfos.get(optionKey),
option, ctx, createTblCmd);
+ } else if
(dataStorageOptionInfos.get(createTblCmd.dataStorage()).containsKey(optionKey))
{
+
processTableOption(dataStorageOptionInfos.get(createTblCmd.dataStorage()).get(optionKey),
option, ctx, createTblCmd);
+ } else {
+ throw new IgniteException(String.format("Unexpected table
option [option=%s, query=%s]", optionKey, ctx.query()));
Review Comment:
Could you please change exception to IgniteInternalException here and below?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -388,104 +428,170 @@ private void ensureSchemaExists(PlanningContext ctx,
String schemaName) {
}
/**
- * Short cut for validating that option value is a simple identifier.
+ * Throws exception with message relates to validation of create table
option.
*
- * @param opt An option to validate.
- * @param ctx Planning context.
+ * @param opt An option which validation was failed.
+ * @param exp A string representing expected values.
+ * @param qry A query the validation was failed for.
*/
- private String paramIsSqlIdentifierValidator(IgniteSqlCreateTableOption
opt, PlanningContext ctx) {
- if (!(opt.value() instanceof SqlIdentifier) || !((SqlIdentifier)
opt.value()).isSimple()) {
- throwOptionParsingException(opt, "a simple identifier",
ctx.query());
+ private static void throwOptionParsingException(IgniteSqlCreateTableOption
opt, String exp, String qry) {
+ throw new IgniteException("Unexpected value for param " + opt.key() +
" ["
+ + "expected " + exp + ", but was " + opt.value() + "; "
+ + "querySql=\"" + qry + "\"]"/*,
IgniteQueryErrorCode.PARSING*/);
+ }
+
+ /**
+ * Collects a mapping of the ID (and ID in quotes) of the data storage to
a name.
+ *
+ * <p>Example: {@code collectDataStorageNames(Set.of("rocksdb"))} ->
{@code Map.of("rocksdb", "rocksdb", "ROCKSDB", "rocksdb")}.
+ *
+ * @param dataStorages Names of the data storages.
+ * @throws IllegalStateException If there is a duplicate ID.
+ */
+ static Map<String, String> collectDataStorageNames(Set<String>
dataStorages) {
+ if (dataStorages.isEmpty()) {
+ return Map.of();
}
- return ((SqlIdentifier) opt.value()).getSimple();
+ Map<String, String> res = new HashMap<>();
+
+ Set<String> ids = new HashSet<>();
+
+ for (String dataStorageName : dataStorages) {
+ addAll(ids, dataStorageName, dataStorageName.toUpperCase());
+
+ for (String id : ids) {
+ if (res.containsKey(id)) {
+ throw new IllegalStateException("Duplicate id:" + id);
+ }
+
+ res.put(id, dataStorageName);
+ }
+
+ ids.clear();
+ }
+
+ return unmodifiableMap(res);
}
/**
- * Creates a validator for an option which value should be value of given
enumeration.
+ * Collects a mapping of the ID (and ID in quotes) of the table option to
a table option info.
+ *
+ * <p>Example for "replicas": {@code Map.of("replicas",
TableOptionInfo@123, "REPLICAS", TableOptionInfo@123)}.
*
- * @param clz Enumeration class to create validator for.
+ * @param tableOptionInfos Table option information's.
+ * @throws IllegalStateException If there is a duplicate ID.
*/
- private static <T extends Enum<T>> BiFunction<IgniteSqlCreateTableOption,
PlanningContext, T> validatorForEnumValue(
- Class<T> clz
- ) {
- return (opt, ctx) -> {
- T val = null;
-
- if (opt.value() instanceof SqlIdentifier) {
- val = Arrays.stream(clz.getEnumConstants())
- .filter(m ->
m.name().equalsIgnoreCase(opt.value().toString()))
- .findFirst()
- .orElse(null);
- }
+ static Map<String, TableOptionInfo<?>>
collectTableOptionInfos(TableOptionInfo<?>... tableOptionInfos) {
+ if (ArrayUtils.nullOrEmpty(tableOptionInfos)) {
+ return Map.of();
+ }
- if (val == null) {
- throwOptionParsingException(opt, "values are "
- + Arrays.toString(clz.getEnumConstants()),
ctx.query());
+ Map<String, TableOptionInfo<?>> res = new HashMap<>();
+
+ Set<String> ids = new HashSet<>();
+
+ for (TableOptionInfo<?> tableOptionInfo : tableOptionInfos) {
+ addAll(ids, tableOptionInfo.name,
tableOptionInfo.name.toUpperCase());
+
+ for (String id : ids) {
+ if (res.containsKey(id)) {
+ throw new IllegalStateException("Duplicate id:" + id);
+ }
+
+ res.put(id, tableOptionInfo);
}
- return val;
- };
+ ids.clear();
+ }
+
+ return unmodifiableMap(res);
}
/**
- * Throws exception with message relates to validation of create table
option.
+ * Checks that there are no ID duplicates.
*
- * @param opt An option which validation was failed.
- * @param exp A string representing expected values.
- * @param qry A query the validation was failed for.
+ * @param tableOptionInfos0 Table options information.
+ * @param tableOptionInfos1 Table options information.
+ * @throws IllegalStateException If there is a duplicate ID.
*/
- private static void throwOptionParsingException(IgniteSqlCreateTableOption
opt, String exp, String qry) {
- throw new IgniteException("Unexpected value for param " + opt.key() +
" ["
- + "expected " + exp + ", but was " + opt.value() + "; "
- + "querySql=\"" + qry + "\"]"/*,
IgniteQueryErrorCode.PARSING*/);
+ static void checkDuplicates(Map<String, TableOptionInfo<?>>
tableOptionInfos0, Map<String, TableOptionInfo<?>> tableOptionInfos1) {
+ for (String id : tableOptionInfos1.keySet()) {
+ if (tableOptionInfos0.containsKey(id)) {
+ throw new IllegalStateException("Duplicate id:" + id);
+ }
+ }
}
- private static class TableOptionProcessor<T> {
- private final IgniteSqlCreateTableOptionEnum key;
-
- private final BiFunction<IgniteSqlCreateTableOption, PlanningContext,
T> validator;
-
- private final BiConsumer<CreateTableCommand, T> valSetter;
-
- /**
- * Constructor.
- *
- * @param key Option key this processor is supopsed to handle.
- * @param validator Validator that derives a value from a {@link
SqlNode}, validates it and then returns if validation passed,
- * throws an exeption otherwise.
- * @param valSetter Setter sets the value recived from the validator
to the given {@link CreateTableCommand}.
- */
- private TableOptionProcessor(
- IgniteSqlCreateTableOptionEnum key,
- BiFunction<IgniteSqlCreateTableOption, PlanningContext, T>
validator,
- BiConsumer<CreateTableCommand, T> valSetter
- ) {
- this.key = key;
- this.validator = validator;
- this.valSetter = valSetter;
+ private String driveDataStorage(@Nullable SqlIdentifier engineName,
PlanningContext ctx) {
Review Comment:
did you mean `derive`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -388,104 +428,170 @@ private void ensureSchemaExists(PlanningContext ctx,
String schemaName) {
}
/**
- * Short cut for validating that option value is a simple identifier.
+ * Throws exception with message relates to validation of create table
option.
*
- * @param opt An option to validate.
- * @param ctx Planning context.
+ * @param opt An option which validation was failed.
+ * @param exp A string representing expected values.
+ * @param qry A query the validation was failed for.
*/
- private String paramIsSqlIdentifierValidator(IgniteSqlCreateTableOption
opt, PlanningContext ctx) {
- if (!(opt.value() instanceof SqlIdentifier) || !((SqlIdentifier)
opt.value()).isSimple()) {
- throwOptionParsingException(opt, "a simple identifier",
ctx.query());
+ private static void throwOptionParsingException(IgniteSqlCreateTableOption
opt, String exp, String qry) {
+ throw new IgniteException("Unexpected value for param " + opt.key() +
" ["
+ + "expected " + exp + ", but was " + opt.value() + "; "
+ + "querySql=\"" + qry + "\"]"/*,
IgniteQueryErrorCode.PARSING*/);
+ }
+
+ /**
+ * Collects a mapping of the ID (and ID in quotes) of the data storage to
a name.
+ *
+ * <p>Example: {@code collectDataStorageNames(Set.of("rocksdb"))} ->
{@code Map.of("rocksdb", "rocksdb", "ROCKSDB", "rocksdb")}.
+ *
+ * @param dataStorages Names of the data storages.
+ * @throws IllegalStateException If there is a duplicate ID.
+ */
+ static Map<String, String> collectDataStorageNames(Set<String>
dataStorages) {
+ if (dataStorages.isEmpty()) {
+ return Map.of();
}
- return ((SqlIdentifier) opt.value()).getSimple();
+ Map<String, String> res = new HashMap<>();
+
+ Set<String> ids = new HashSet<>();
+
+ for (String dataStorageName : dataStorages) {
+ addAll(ids, dataStorageName, dataStorageName.toUpperCase());
+
+ for (String id : ids) {
+ if (res.containsKey(id)) {
+ throw new IllegalStateException("Duplicate id:" + id);
+ }
+
+ res.put(id, dataStorageName);
+ }
+
+ ids.clear();
+ }
+
+ return unmodifiableMap(res);
}
/**
- * Creates a validator for an option which value should be value of given
enumeration.
+ * Collects a mapping of the ID (and ID in quotes) of the table option to
a table option info.
+ *
+ * <p>Example for "replicas": {@code Map.of("replicas",
TableOptionInfo@123, "REPLICAS", TableOptionInfo@123)}.
*
- * @param clz Enumeration class to create validator for.
+ * @param tableOptionInfos Table option information's.
+ * @throws IllegalStateException If there is a duplicate ID.
*/
- private static <T extends Enum<T>> BiFunction<IgniteSqlCreateTableOption,
PlanningContext, T> validatorForEnumValue(
- Class<T> clz
- ) {
- return (opt, ctx) -> {
- T val = null;
-
- if (opt.value() instanceof SqlIdentifier) {
- val = Arrays.stream(clz.getEnumConstants())
- .filter(m ->
m.name().equalsIgnoreCase(opt.value().toString()))
- .findFirst()
- .orElse(null);
- }
+ static Map<String, TableOptionInfo<?>>
collectTableOptionInfos(TableOptionInfo<?>... tableOptionInfos) {
+ if (ArrayUtils.nullOrEmpty(tableOptionInfos)) {
+ return Map.of();
+ }
- if (val == null) {
- throwOptionParsingException(opt, "values are "
- + Arrays.toString(clz.getEnumConstants()),
ctx.query());
+ Map<String, TableOptionInfo<?>> res = new HashMap<>();
+
+ Set<String> ids = new HashSet<>();
+
+ for (TableOptionInfo<?> tableOptionInfo : tableOptionInfos) {
+ addAll(ids, tableOptionInfo.name,
tableOptionInfo.name.toUpperCase());
Review Comment:
why do we need to support both variants? Does it make sense to make these
properties case insensitive?
--
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]
