You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korlov42 (via GitHub)" <gi...@apache.org> on 2023/06/16 06:44:15 UTC
[GitHub] [ignite-3] korlov42 opened a new pull request, #2203: remove class Columns
korlov42 opened a new pull request, #2203:
URL: https://github.com/apache/ignite-3/pull/2203
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519390206
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
Review Comment:
This can be an utility method.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519390206
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
Review Comment:
This can be an utility method.
Validation can be placed into a separate ` if (isAssertionEnabled)` block, or even throw an exception.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519546475
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -115,15 +80,7 @@ public class TableRowConverterImpl implements TableRowConverter {
public <RowT> BinaryRowEx toBinaryRow(ExecutionContext<RowT> ectx, RowT row, boolean key) {
BinaryTuple binaryTuple = ectx.rowHandler().toBinaryTuple(row);
- if (!key) {
- InternalTuple tuple = schemaDescriptor.physicalOrderMatchesLogical()
- ? binaryTuple
- : new FormatAwareProjectedTuple(binaryTuple, fullMapping);
-
- return SqlOutputBinaryRow.newRow(schemaDescriptor.version(), fullRowColocationIndexes, colocationColumnTypes, tuple);
- } else {
- return SqlOutputBinaryRow.newRow(schemaDescriptor.version(), keyOnlyColocationIndexes, colocationColumnTypes, binaryTuple);
- }
+ return SqlOutputBinaryRow.newRow(schemaDescriptor, key, binaryTuple);
Review Comment:
Is it possible to get rid of `boolean key` flag?
Having a special method usually better than using a flag, when the caller always knows the case.
It is ok to do in a separate ticket.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519867721
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -115,15 +80,7 @@ public class TableRowConverterImpl implements TableRowConverter {
public <RowT> BinaryRowEx toBinaryRow(ExecutionContext<RowT> ectx, RowT row, boolean key) {
BinaryTuple binaryTuple = ectx.rowHandler().toBinaryTuple(row);
- if (!key) {
- InternalTuple tuple = schemaDescriptor.physicalOrderMatchesLogical()
- ? binaryTuple
- : new FormatAwareProjectedTuple(binaryTuple, fullMapping);
-
- return SqlOutputBinaryRow.newRow(schemaDescriptor.version(), fullRowColocationIndexes, colocationColumnTypes, tuple);
- } else {
- return SqlOutputBinaryRow.newRow(schemaDescriptor.version(), keyOnlyColocationIndexes, colocationColumnTypes, binaryTuple);
- }
+ return SqlOutputBinaryRow.newRow(schemaDescriptor, key, binaryTuple);
Review Comment:
make sense. I filed an issue https://issues.apache.org/jira/browse/IGNITE-21731
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1520009192
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
Review Comment:
the goal of this patch is to remove code rather than adding new one, so I tried not add new lines until it's absolutely required in order to proceed. There was no validation before, but after refactoring this object become more sensible to unspoken invariants like "no duplicate columns". In order to find and all the places that violates such invariant I've added bunch of assertions.
I think, the proper fix of validation should be done under separate issue (https://issues.apache.org/jira/browse/IGNITE-21733).
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
- this.ver = ver;
- this.keyCols = new Columns(0, keyCols);
- this.valCols = new Columns(keyCols.length, valCols);
-
- this.columns = Stream.concat(
- Arrays.stream(this.keyCols.columns()),
- Arrays.stream(this.valCols.columns())
- ).collect(Collectors.toList());
-
- assert this.keyCols.nullMapSize() == 0 : "Primary key cannot contain nullable column [cols=" + this.keyCols + ']';
-
- colMap = newLinkedHashMap(keyCols.length + valCols.length);
- var hasTemporalColumns = new AtomicBoolean(false);
-
- Stream.concat(Arrays.stream(this.keyCols.columns()), Arrays.stream(this.valCols.columns()))
- .sorted(Comparator.comparingInt(Column::columnOrder))
- .forEach(c -> {
- if (c.type() instanceof TemporalNativeType) {
- hasTemporalColumns.set(true);
- }
-
- colMap.put(c.name(), c);
- });
-
- this.physicalOrderMatchesLogical = colMap.values().stream().allMatch(col -> col.columnOrder() == col.schemaIndex());
-
- this.hasTemporalColumns = hasTemporalColumns.get();
-
- // Preserving key chunk column order is not actually required.
- // It is sufficient to has same column order for all nodes.
- if (ArrayUtils.nullOrEmpty(colocationCols)) {
- this.colocationCols = this.keyCols.columns();
- this.colocationColIndexes = null;
- } else {
- this.colocationCols = new Column[colocationCols.length];
- this.colocationColIndexes = new HashMap<>(colocationCols.length);
-
- for (int i = 0; i < colocationCols.length; i++) {
- Column col = colMap.get(colocationCols[i]);
- this.colocationCols[i] = col;
- this.colocationColIndexes.put(col, i);
+ if (colocationColumns == null) {
+ colocationColumns = keyColumns;
+ }
+
+ Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
Review Comment:
fixed, thanks
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519923007
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -46,66 +41,36 @@ public class TableRowConverterImpl implements TableRowConverter {
private final BinaryTupleSchema fullTupleSchema;
- private final int[] fullRowColocationIndexes;
- private final int[] keyOnlyColocationIndexes;
- private final NativeType[] colocationColumnTypes;
-
/**
* Mapping of required columns to their indexes in physical schema.
*/
private final int[] requiredColumnsMapping;
- /**
- * Mapping of all columns to their indexes in physical schema.
- */
- private final int[] fullMapping;
-
- private final boolean skipReshuffling;
+ private final boolean skipTrimming;
/** Constructor. */
TableRowConverterImpl(
SchemaRegistry schemaRegistry,
BinaryTupleSchema fullTupleSchema,
- int[] fullRowColocationIndexes,
- int[] keyOnlyColocationIndexes,
- NativeType[] colocationColumnTypes,
SchemaDescriptor schemaDescriptor,
@Nullable BitSet requiredColumns
) {
this.schemaRegistry = schemaRegistry;
this.schemaDescriptor = schemaDescriptor;
- this.fullRowColocationIndexes = fullRowColocationIndexes;
- this.keyOnlyColocationIndexes = keyOnlyColocationIndexes;
- this.colocationColumnTypes = colocationColumnTypes;
this.fullTupleSchema = fullTupleSchema;
- this.skipReshuffling = requiredColumns == null && schemaDescriptor.physicalOrderMatchesLogical();
+ this.skipTrimming = requiredColumns == null;
Review Comment:
good point! Filed an issue https://issues.apache.org/jira/browse/IGNITE-21732
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519806797
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
- this.ver = ver;
- this.keyCols = new Columns(0, keyCols);
- this.valCols = new Columns(keyCols.length, valCols);
-
- this.columns = Stream.concat(
- Arrays.stream(this.keyCols.columns()),
- Arrays.stream(this.valCols.columns())
- ).collect(Collectors.toList());
-
- assert this.keyCols.nullMapSize() == 0 : "Primary key cannot contain nullable column [cols=" + this.keyCols + ']';
-
- colMap = newLinkedHashMap(keyCols.length + valCols.length);
- var hasTemporalColumns = new AtomicBoolean(false);
-
- Stream.concat(Arrays.stream(this.keyCols.columns()), Arrays.stream(this.valCols.columns()))
- .sorted(Comparator.comparingInt(Column::columnOrder))
- .forEach(c -> {
- if (c.type() instanceof TemporalNativeType) {
- hasTemporalColumns.set(true);
- }
-
- colMap.put(c.name(), c);
- });
-
- this.physicalOrderMatchesLogical = colMap.values().stream().allMatch(col -> col.columnOrder() == col.schemaIndex());
-
- this.hasTemporalColumns = hasTemporalColumns.get();
-
- // Preserving key chunk column order is not actually required.
- // It is sufficient to has same column order for all nodes.
- if (ArrayUtils.nullOrEmpty(colocationCols)) {
- this.colocationCols = this.keyCols.columns();
- this.colocationColIndexes = null;
- } else {
- this.colocationCols = new Column[colocationCols.length];
- this.colocationColIndexes = new HashMap<>(colocationCols.length);
-
- for (int i = 0; i < colocationCols.length; i++) {
- Column col = colMap.get(colocationCols[i]);
- this.colocationCols[i] = col;
- this.colocationColIndexes.put(col, i);
+ if (colocationColumns == null) {
+ colocationColumns = keyColumns;
+ }
+
+ Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
+ idx = 0;
+ for (String name : colocationColumns) {
+ assert columnNameToPositionInKey.containsKey(name)
+ : "Colocation column must be part of the key: " + name;
+ assert !columnNameToPositionInColocation.containsKey(name)
+ : "Colocation column must not have duplicates: " + name;
+
+ columnNameToPositionInColocation.put(name, idx++);
+ }
+
+ boolean hasTemporalColumns = false;
+ int rowPosition = 0;
+ int valuePosition = 0;
+ for (Column column : columns) {
+ Column orderedColumn = column.copy(
+ rowPosition++,
+ columnNameToPositionInKey.getOrDefault(column.name(), -1),
+ columnNameToPositionInKey.containsKey(column.name()) ? -1 : valuePosition++,
+ columnNameToPositionInColocation.getOrDefault(column.name(), -1)
+ );
+
+ Column old = columnsByName.put(orderedColumn.name(), orderedColumn);
+
+ assert old == null : "Columns with similar names are not allowed: " + old.name();
+
+ orderedColumns.add(orderedColumn);
+
+ if (column.type() instanceof TemporalNativeType) {
+ hasTemporalColumns = true;
}
}
- // TODO: Move keyIndex and colocationIndex to Column class for faster and simpler access?
- keyColIndexes = new HashMap<>(keyCols.length);
+ this.ver = ver;
+ this.columns = List.copyOf(orderedColumns);
+ this.columnsByName = Map.copyOf(columnsByName);
Review Comment:
according to javadoc, `Map.copyOf` returns unmodifiable Map
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519835644
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
- this.ver = ver;
- this.keyCols = new Columns(0, keyCols);
- this.valCols = new Columns(keyCols.length, valCols);
-
- this.columns = Stream.concat(
- Arrays.stream(this.keyCols.columns()),
- Arrays.stream(this.valCols.columns())
- ).collect(Collectors.toList());
-
- assert this.keyCols.nullMapSize() == 0 : "Primary key cannot contain nullable column [cols=" + this.keyCols + ']';
-
- colMap = newLinkedHashMap(keyCols.length + valCols.length);
- var hasTemporalColumns = new AtomicBoolean(false);
-
- Stream.concat(Arrays.stream(this.keyCols.columns()), Arrays.stream(this.valCols.columns()))
- .sorted(Comparator.comparingInt(Column::columnOrder))
- .forEach(c -> {
- if (c.type() instanceof TemporalNativeType) {
- hasTemporalColumns.set(true);
- }
-
- colMap.put(c.name(), c);
- });
-
- this.physicalOrderMatchesLogical = colMap.values().stream().allMatch(col -> col.columnOrder() == col.schemaIndex());
-
- this.hasTemporalColumns = hasTemporalColumns.get();
-
- // Preserving key chunk column order is not actually required.
- // It is sufficient to has same column order for all nodes.
- if (ArrayUtils.nullOrEmpty(colocationCols)) {
- this.colocationCols = this.keyCols.columns();
- this.colocationColIndexes = null;
- } else {
- this.colocationCols = new Column[colocationCols.length];
- this.colocationColIndexes = new HashMap<>(colocationCols.length);
-
- for (int i = 0; i < colocationCols.length; i++) {
- Column col = colMap.get(colocationCols[i]);
- this.colocationCols[i] = col;
- this.colocationColIndexes.put(col, i);
+ if (colocationColumns == null) {
+ colocationColumns = keyColumns;
+ }
+
+ Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
+ idx = 0;
+ for (String name : colocationColumns) {
+ assert columnNameToPositionInKey.containsKey(name)
+ : "Colocation column must be part of the key: " + name;
+ assert !columnNameToPositionInColocation.containsKey(name)
+ : "Colocation column must not have duplicates: " + name;
+
+ columnNameToPositionInColocation.put(name, idx++);
+ }
+
+ boolean hasTemporalColumns = false;
+ int rowPosition = 0;
+ int valuePosition = 0;
+ for (Column column : columns) {
+ Column orderedColumn = column.copy(
+ rowPosition++,
+ columnNameToPositionInKey.getOrDefault(column.name(), -1),
+ columnNameToPositionInKey.containsKey(column.name()) ? -1 : valuePosition++,
+ columnNameToPositionInColocation.getOrDefault(column.name(), -1)
+ );
+
+ Column old = columnsByName.put(orderedColumn.name(), orderedColumn);
+
+ assert old == null : "Columns with similar names are not allowed: " + old.name();
+
+ orderedColumns.add(orderedColumn);
+
+ if (column.type() instanceof TemporalNativeType) {
+ hasTemporalColumns = true;
}
}
- // TODO: Move keyIndex and colocationIndex to Column class for faster and simpler access?
- keyColIndexes = new HashMap<>(keyCols.length);
+ this.ver = ver;
+ this.columns = List.copyOf(orderedColumns);
+ this.columnsByName = Map.copyOf(columnsByName);
Review Comment:
But it's create a copy of collection.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519391494
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
- this.ver = ver;
- this.keyCols = new Columns(0, keyCols);
- this.valCols = new Columns(keyCols.length, valCols);
-
- this.columns = Stream.concat(
- Arrays.stream(this.keyCols.columns()),
- Arrays.stream(this.valCols.columns())
- ).collect(Collectors.toList());
-
- assert this.keyCols.nullMapSize() == 0 : "Primary key cannot contain nullable column [cols=" + this.keyCols + ']';
-
- colMap = newLinkedHashMap(keyCols.length + valCols.length);
- var hasTemporalColumns = new AtomicBoolean(false);
-
- Stream.concat(Arrays.stream(this.keyCols.columns()), Arrays.stream(this.valCols.columns()))
- .sorted(Comparator.comparingInt(Column::columnOrder))
- .forEach(c -> {
- if (c.type() instanceof TemporalNativeType) {
- hasTemporalColumns.set(true);
- }
-
- colMap.put(c.name(), c);
- });
-
- this.physicalOrderMatchesLogical = colMap.values().stream().allMatch(col -> col.columnOrder() == col.schemaIndex());
-
- this.hasTemporalColumns = hasTemporalColumns.get();
-
- // Preserving key chunk column order is not actually required.
- // It is sufficient to has same column order for all nodes.
- if (ArrayUtils.nullOrEmpty(colocationCols)) {
- this.colocationCols = this.keyCols.columns();
- this.colocationColIndexes = null;
- } else {
- this.colocationCols = new Column[colocationCols.length];
- this.colocationColIndexes = new HashMap<>(colocationCols.length);
-
- for (int i = 0; i < colocationCols.length; i++) {
- Column col = colMap.get(colocationCols[i]);
- this.colocationCols[i] = col;
- this.colocationColIndexes.put(col, i);
+ if (colocationColumns == null) {
+ colocationColumns = keyColumns;
+ }
+
+ Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
Review Comment:
```suggestion
if (colocationColumns == null) {
columnNameToPositionInColocation = columnNameToPositionInKey;
}
else {
Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
```
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519532335
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -46,66 +41,36 @@ public class TableRowConverterImpl implements TableRowConverter {
private final BinaryTupleSchema fullTupleSchema;
- private final int[] fullRowColocationIndexes;
- private final int[] keyOnlyColocationIndexes;
- private final NativeType[] colocationColumnTypes;
-
/**
* Mapping of required columns to their indexes in physical schema.
*/
private final int[] requiredColumnsMapping;
- /**
- * Mapping of all columns to their indexes in physical schema.
- */
- private final int[] fullMapping;
-
- private final boolean skipReshuffling;
+ private final boolean skipTrimming;
/** Constructor. */
TableRowConverterImpl(
SchemaRegistry schemaRegistry,
BinaryTupleSchema fullTupleSchema,
- int[] fullRowColocationIndexes,
- int[] keyOnlyColocationIndexes,
- NativeType[] colocationColumnTypes,
SchemaDescriptor schemaDescriptor,
@Nullable BitSet requiredColumns
) {
this.schemaRegistry = schemaRegistry;
this.schemaDescriptor = schemaDescriptor;
- this.fullRowColocationIndexes = fullRowColocationIndexes;
- this.keyOnlyColocationIndexes = keyOnlyColocationIndexes;
- this.colocationColumnTypes = colocationColumnTypes;
this.fullTupleSchema = fullTupleSchema;
- this.skipReshuffling = requiredColumns == null && schemaDescriptor.physicalOrderMatchesLogical();
+ this.skipTrimming = requiredColumns == null;
Review Comment:
Maybe it is better make the class package-private and have 2 constructor for both cases: `requiredColumns` is null and not null; instead of having bunch of `if (requiredColumns == null)`? Or maybe different implementations?
In fact, the object is always created via factory and it is clear which constructor must be used.
It is ok to make a refactoring in a. separate ticket.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519397243
##########
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaDescriptor.java:
##########
@@ -84,71 +82,118 @@ public class SchemaDescriptor {
* @param keyCols Key columns.
* @param valCols Value columns.
*/
+ @TestOnly
public SchemaDescriptor(int ver, Column[] keyCols, Column[] valCols) {
- this(ver, keyCols, null, valCols);
+ this(
+ ver,
+ mergeColumns(keyCols, valCols),
+ Arrays.stream(keyCols).map(Column::name).collect(Collectors.toList()),
+ null
+ );
}
- /**
- * Constructor.
- *
- * @param ver Schema version.
- * @param keyCols Key columns.
- * @param colocationCols Colocation column names.
- * @param valCols Value columns.
- */
- public SchemaDescriptor(int ver, Column[] keyCols, String @Nullable[] colocationCols, Column[] valCols) {
- assert keyCols.length > 0 : "No key columns are configured.";
+ /** Constructor. */
+ public SchemaDescriptor(
+ int ver,
+ List<Column> columns,
+ List<String> keyColumns,
+ @Nullable List<String> colocationColumns
+ ) {
+ assert !nullOrEmpty(columns) : "Schema should have at least one column";
+
+ Map<String, Column> columnsByName = new HashMap<>();
+ List<Column> orderedColumns = new ArrayList<>(columns.size());
+
+ Object2IntMap<String> columnNameToPositionInKey = new Object2IntOpenHashMap<>();
+ int idx = 0;
+ for (String name : keyColumns) {
+ assert !columnNameToPositionInKey.containsKey(name)
+ : "Key column must not have duplicates: " + name;
+
+ columnNameToPositionInKey.put(name, idx++);
+ }
- this.ver = ver;
- this.keyCols = new Columns(0, keyCols);
- this.valCols = new Columns(keyCols.length, valCols);
-
- this.columns = Stream.concat(
- Arrays.stream(this.keyCols.columns()),
- Arrays.stream(this.valCols.columns())
- ).collect(Collectors.toList());
-
- assert this.keyCols.nullMapSize() == 0 : "Primary key cannot contain nullable column [cols=" + this.keyCols + ']';
-
- colMap = newLinkedHashMap(keyCols.length + valCols.length);
- var hasTemporalColumns = new AtomicBoolean(false);
-
- Stream.concat(Arrays.stream(this.keyCols.columns()), Arrays.stream(this.valCols.columns()))
- .sorted(Comparator.comparingInt(Column::columnOrder))
- .forEach(c -> {
- if (c.type() instanceof TemporalNativeType) {
- hasTemporalColumns.set(true);
- }
-
- colMap.put(c.name(), c);
- });
-
- this.physicalOrderMatchesLogical = colMap.values().stream().allMatch(col -> col.columnOrder() == col.schemaIndex());
-
- this.hasTemporalColumns = hasTemporalColumns.get();
-
- // Preserving key chunk column order is not actually required.
- // It is sufficient to has same column order for all nodes.
- if (ArrayUtils.nullOrEmpty(colocationCols)) {
- this.colocationCols = this.keyCols.columns();
- this.colocationColIndexes = null;
- } else {
- this.colocationCols = new Column[colocationCols.length];
- this.colocationColIndexes = new HashMap<>(colocationCols.length);
-
- for (int i = 0; i < colocationCols.length; i++) {
- Column col = colMap.get(colocationCols[i]);
- this.colocationCols[i] = col;
- this.colocationColIndexes.put(col, i);
+ if (colocationColumns == null) {
+ colocationColumns = keyColumns;
+ }
+
+ Object2IntMap<String> columnNameToPositionInColocation = new Object2IntOpenHashMap<>();
+ idx = 0;
+ for (String name : colocationColumns) {
+ assert columnNameToPositionInKey.containsKey(name)
+ : "Colocation column must be part of the key: " + name;
+ assert !columnNameToPositionInColocation.containsKey(name)
+ : "Colocation column must not have duplicates: " + name;
+
+ columnNameToPositionInColocation.put(name, idx++);
+ }
+
+ boolean hasTemporalColumns = false;
+ int rowPosition = 0;
+ int valuePosition = 0;
+ for (Column column : columns) {
+ Column orderedColumn = column.copy(
+ rowPosition++,
+ columnNameToPositionInKey.getOrDefault(column.name(), -1),
+ columnNameToPositionInKey.containsKey(column.name()) ? -1 : valuePosition++,
+ columnNameToPositionInColocation.getOrDefault(column.name(), -1)
+ );
+
+ Column old = columnsByName.put(orderedColumn.name(), orderedColumn);
+
+ assert old == null : "Columns with similar names are not allowed: " + old.name();
+
+ orderedColumns.add(orderedColumn);
+
+ if (column.type() instanceof TemporalNativeType) {
+ hasTemporalColumns = true;
}
}
- // TODO: Move keyIndex and colocationIndex to Column class for faster and simpler access?
- keyColIndexes = new HashMap<>(keyCols.length);
+ this.ver = ver;
+ this.columns = List.copyOf(orderedColumns);
+ this.columnsByName = Map.copyOf(columnsByName);
Review Comment:
Can a copy be replaced with immutable collection?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203#discussion_r1519532335
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -46,66 +41,36 @@ public class TableRowConverterImpl implements TableRowConverter {
private final BinaryTupleSchema fullTupleSchema;
- private final int[] fullRowColocationIndexes;
- private final int[] keyOnlyColocationIndexes;
- private final NativeType[] colocationColumnTypes;
-
/**
* Mapping of required columns to their indexes in physical schema.
*/
private final int[] requiredColumnsMapping;
- /**
- * Mapping of all columns to their indexes in physical schema.
- */
- private final int[] fullMapping;
-
- private final boolean skipReshuffling;
+ private final boolean skipTrimming;
/** Constructor. */
TableRowConverterImpl(
SchemaRegistry schemaRegistry,
BinaryTupleSchema fullTupleSchema,
- int[] fullRowColocationIndexes,
- int[] keyOnlyColocationIndexes,
- NativeType[] colocationColumnTypes,
SchemaDescriptor schemaDescriptor,
@Nullable BitSet requiredColumns
) {
this.schemaRegistry = schemaRegistry;
this.schemaDescriptor = schemaDescriptor;
- this.fullRowColocationIndexes = fullRowColocationIndexes;
- this.keyOnlyColocationIndexes = keyOnlyColocationIndexes;
- this.colocationColumnTypes = colocationColumnTypes;
this.fullTupleSchema = fullTupleSchema;
- this.skipReshuffling = requiredColumns == null && schemaDescriptor.physicalOrderMatchesLogical();
+ this.skipTrimming = requiredColumns == null;
Review Comment:
Maybe it is better make the class package-private and have 2 constructor for both cases: `requiredColumns` is null and not null; instead of having bunch of `if (requiredColumns == null)`? Or maybe different implementations?
In fact, the object is always created via factory and it is clear which constructor must be used.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-19744 Clean up IEP-54 leftovers [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #2203:
URL: https://github.com/apache/ignite-3/pull/2203
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org