korlov42 commented on a change in pull request #201:
URL: https://github.com/apache/ignite-3/pull/201#discussion_r672153442
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaManager.java
##########
@@ -283,9 +285,9 @@ private ColumnMapper columnMapper(
continue;
if (mapper == null)
- mapper = ColumnMapping.mapperBuilder(newDesc.length());
+ mapper = ColumnMapping.mapperBuilder(newDesc);
- mapper.add(newCol.schemaIndex(), oldCol.schemaIndex());
+ mapper.add(newCol.schemaIndex(), oldCol.schemaIndex(), null);
Review comment:
why don't you pass `oldCol` as 3rd argument?
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/mapping/ColumnMapper.java
##########
@@ -30,4 +31,12 @@
* @return Column index in target schema or {@code -1} if no column exists
in target schema.
*/
int map(int idx);
+
+ /**
+ * Column descriptor for given column idx.
+ *
+ * @param idx Column index in source schema.
+ * @return Column descriptor.
+ */
+ Column mappedColumn(int idx);
Review comment:
let's denote a return type as `@Nullable`
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/mapping/ColumnMapperBuilder.java
##########
@@ -17,17 +17,31 @@
package org.apache.ignite.internal.schema.mapping;
+import org.apache.ignite.internal.schema.Column;
+import org.jetbrains.annotations.NotNull;
+import org.jetbrains.annotations.Nullable;
+
/**
* Column mapper builder interface.
*/
public interface ColumnMapperBuilder {
/**
- * Add column mapping.
+ * Add new column.
+ *
+ * @param col Column descriptor.
+ * @return {@code this} for chaining.
+ */
+ public ColumnMapperBuilder add(@NotNull Column col);
+
+ /**
+ * Remap column.
*
* @param from Source column index.
* @param to Target column index.
+ * @param col Target column descriptor.
Review comment:
let's describe when null is allowed for `col`
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/mapping/ColumnMapperImpl.java
##########
@@ -17,34 +17,56 @@
package org.apache.ignite.internal.schema.mapping;
+import org.apache.ignite.internal.schema.Column;
+import org.apache.ignite.internal.schema.SchemaDescriptor;
+import org.jetbrains.annotations.NotNull;
+
/**
* Column mapper implementation.
*/
class ColumnMapperImpl implements ColumnMapper, ColumnMapperBuilder {
/** Mapping. */
private final int[] mapping;
+ /** Mapped columns. */
+ private final Column[] cols;
+
/**
- * @param cols Number of columns.
+ * @param schema Schema descriptor.
*/
- ColumnMapperImpl(int cols) {
- mapping = new int[cols];
+ ColumnMapperImpl(SchemaDescriptor schema) {
+ mapping = new int[schema.length()];
+ cols = new Column[schema.length()];
- for (int i = 0; i < mapping.length; i++)
+ for (int i = 0; i < mapping.length; i++) {
mapping[i] = i;
+ cols[i] = schema.column(i);
+ }
}
/** {@inheritDoc} */
- @Override public void add(int from, int to) {
+ @Override public ColumnMapperImpl add(@NotNull Column col) {
+ add(col.schemaIndex(), -1, col);
Review comment:
this invocation violates contract from javadoc: `col` is a target column
descriptor, but if there is no such column in target (to==-1), then there
should not be any descriptor
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/mapping/ColumnMapperImpl.java
##########
@@ -17,34 +17,56 @@
package org.apache.ignite.internal.schema.mapping;
+import org.apache.ignite.internal.schema.Column;
+import org.apache.ignite.internal.schema.SchemaDescriptor;
+import org.jetbrains.annotations.NotNull;
+
/**
* Column mapper implementation.
*/
class ColumnMapperImpl implements ColumnMapper, ColumnMapperBuilder {
Review comment:
I would prefer either split this onto two different classes or merge
ColumnMapper and ColumnMapperBuilder interfaces. The builder pattern is used
usually to create an object, not to change its state.
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/registry/UpgradingRowAdapter.java
##########
@@ -31,21 +36,339 @@
/** Column mapper. */
private final ColumnMapper mapper;
+ /** Adapter schema. */
+ private final SchemaDescriptor schema;
+
/**
- * @param schema Schema descriptor of new version.
+ * @param schema Row adapter schema descriptor.
+ * @param rowSchema Row schema descriptor.
* @param row Row.
* @param mapper Column mapper.
*/
- UpgradingRowAdapter(SchemaDescriptor schema, BinaryRow row, ColumnMapper
mapper) {
- super(schema, row);
+ UpgradingRowAdapter(SchemaDescriptor schema, SchemaDescriptor rowSchema,
BinaryRow row, ColumnMapper mapper) {
+ super(rowSchema, row);
+ this.schema = schema;
this.mapper = mapper;
}
/** {@inheritDoc} */
- @Override protected long findColumn(int colIdx, NativeTypeSpec type)
throws InvalidTypeException {
- int mapIdx = mapper.map(colIdx);
+ @Override public SchemaDescriptor rowSchema() {
+ return schema;
+ }
+
+ /** {@inheritDoc} */
+ @Override public int schemaVersion() {
+ return schema.version();
+ }
+
+ /**
+ * Map column.
+ *
+ * @param colIdx Column index in source schema.
+ * @return Column index in targer schema.
+ */
+ private int mapColumn(int colIdx) throws InvalidTypeException {
+ return mapper.map(colIdx);
+ }
+
+ /**
+ * Reads value for specified column.
+ *
+ * @param colIdx Column index.
+ * @return Column value.
+ * @throws InvalidTypeException If actual column type does not match the
requested column type.
+ */
+ @Override public byte byteValue(int colIdx) throws InvalidTypeException {
+ int mappedId = mapColumn(colIdx);
+
+ Column column = mappedId < 0 ? mapper.mappedColumn(colIdx) :
super.rowSchema().column(mappedId);
Review comment:
looks like this could be simplified to `Column column =
mapper.mappedColumn(colIdx)`
--
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]
