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/09/05 12:11:09 UTC
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2512: IGNITE-19791 Introduce SqlRowHandler
korlov42 commented on code in PR #2512:
URL: https://github.com/apache/ignite-3/pull/2512#discussion_r1315729337
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/RowHandler.java:
##########
@@ -87,5 +105,13 @@ interface RowFactory<RowT> {
* @return Instantiation defined representation.
*/
RowT create(ByteBuffer raw);
+
+ /**
+ * Wraps incoming binary tuple into a instantiation defined representation.
+ *
+ * @param tuple {@link InternalTuple} representation.
+ * @return Instantiation defined representation.
+ */
+ RowT wrap(InternalTuple tuple);
Review Comment:
let's call it `create` to be consistent with rest of the methods
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/RowHandler.java:
##########
@@ -44,6 +45,23 @@ public interface RowHandler<RowT> {
/** Concatenate two rows. */
RowT concat(RowT left, RowT right);
+ /**
+ * Creates a new row containing only the fields specified in the provided mapping.
+ *
+ * <p>For example:
+ * <pre>
+ * source row [5, 6, 7, 8] apply mapping [0, 2] with offset 1
+ * result mapping will be [1, 3]
+ * result row will be [6, 8]
+ * </pre>
+ *
+ * @param row Source row.
+ * @param mapping Target field indexes.
+ * @param offset Target field indexes offset.
+ * @return A new row with fields from the specified mapping.
+ */
+ RowT map(RowT row, int[] mapping, int offset);
Review Comment:
Firstly, this method has rather peculiar signature. Why do we need offset here? An array of integers is actually all you need to create a projection of original row.
Secondly, why do we even need this method in the first place? I see only one usage, and there it does make sense to create new row explicitly and fill it with proper values
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/row/RowSchema.java:
##########
@@ -39,6 +43,43 @@ public List<TypeSpec> fields() {
return fields;
}
+ /**
+ * Gets an object representation from a tuple's field.
+ *
+ * @param tuple Tuple to read the value from.
+ * @param fieldIndex Field index to read.
+ * @return An object representation of the value.
+ */
+ public @Nullable Object value(InternalTuple tuple, int fieldIndex) {
Review Comment:
RowSchema definitely not the proper place to put this method to
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/SqlRowHandler.java:
##########
@@ -0,0 +1,272 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.sql.engine.exec;
+
+import java.nio.ByteBuffer;
+import java.util.List;
+import org.apache.ignite.internal.schema.BinaryTuple;
+import org.apache.ignite.internal.schema.NativeType;
+import org.apache.ignite.internal.schema.row.InternalTuple;
+import org.apache.ignite.internal.sql.engine.exec.SqlRowHandler.RowWrapper;
+import org.apache.ignite.internal.sql.engine.exec.row.RowSchema;
+import org.apache.ignite.internal.sql.engine.exec.row.RowSchema.Builder;
+import org.apache.ignite.internal.sql.engine.exec.row.RowSchemaTypes;
+import org.apache.ignite.internal.sql.engine.exec.row.TypeSpec;
+import org.apache.ignite.internal.sql.engine.util.Commons;
+import org.apache.ignite.internal.sql.engine.util.TypeUtils;
+import org.apache.ignite.lang.IgniteStringBuilder;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Handler that uses a {@link RowWrapper wrapper} to operate with two types of row implementations.
+ *
+ * <ul>
+ * <li>{@link ObjectsArrayRowWrapper Objects array}</li>
+ * <li>{@link BinaryTupleRowWrapper Binary tuple}</li>
+ * </ul>
+ *
+ * <p>Each kind of rows is serialized to the same binary tuple format
+ * using the {@link #toByteBuffer(RowWrapper) toByteBuffer} method.
+ *
+ * <p>Factory methods {@link RowFactory#wrap(InternalTuple) wrap(InternalTuple)} and
+ * {@link RowFactory#create(ByteBuffer) create(ByteBuffer)} allow create rows without
+ * any additional conversions. But the fields in binary tuple must match the
+ * factory {@link RowSchema row schema}.
+ */
+public class SqlRowHandler implements RowHandler<RowWrapper> {
+ public static final RowHandler<RowWrapper> INSTANCE = new SqlRowHandler();
+
+ private SqlRowHandler() {
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public @Nullable Object get(int field, RowWrapper row) {
+ return row.get(field);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public void set(int field, RowWrapper row, @Nullable Object val) {
+ row.set(field, val);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper concat(RowWrapper left, RowWrapper right) {
+ int leftLen = left.columnsCount();
+ int rightLen = right.columnsCount();
+ List<TypeSpec> leftTypes = left.rowSchema().fields();
+ List<TypeSpec> rightTypes = right.rowSchema().fields();
+
+ Object[] values = new Object[leftLen + rightLen];
+ Builder schemaBuilder = RowSchema.builder();
+
+ for (int i = 0; i < leftLen; i++) {
+ values[i] = left.get(i);
+ schemaBuilder.addField(leftTypes.get(i));
+ }
+
+ for (int i = 0; i < rightLen; i++) {
+ values[leftLen + i] = right.get(i);
+ schemaBuilder.addField(rightTypes.get(i));
+ }
+
+ return new ObjectsArrayRowWrapper(schemaBuilder.build(), values);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper map(RowWrapper row, int[] mapping, int offset) {
+ Object[] fields = new Object[mapping.length];
+
+ for (int i = 0; i < mapping.length; i++) {
+ fields[i] = row.get(mapping[i] + offset);
+ }
+
+ return new ObjectsArrayRowWrapper(row.rowSchema(), fields);
+ }
+
+ @Override
+ public int columnCount(RowWrapper row) {
+ return row.columnsCount();
+ }
+
+ @Override
+ public String toString(RowWrapper row) {
+ IgniteStringBuilder buf = new IgniteStringBuilder("Row[");
+ int maxIdx = columnCount(row) - 1;
+
+ for (int i = 0; i <= maxIdx; i++) {
+ buf.app(row.get(i));
+
+ if (i != maxIdx) {
+ buf.app(", ");
+ }
+ }
+
+ return buf.app(']').toString();
+ }
+
+ @Override
+ public ByteBuffer toByteBuffer(RowWrapper row) {
+ return row.toByteBuffer();
+ }
+
+ @Override
+ public RowFactory<RowWrapper> factory(RowSchema rowSchema) {
+ int rowLen = rowSchema.fields().size();
+
+ return new RowFactory<>() {
+ /** {@inheritDoc} */
+ @Override
+ public RowHandler<RowWrapper> handler() {
+ return SqlRowHandler.this;
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper create() {
+ return create(new Object[rowLen]);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper create(Object... fields) {
+ assert fields.length == rowLen;
+
+ return new ObjectsArrayRowWrapper(rowSchema, fields);
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper create(ByteBuffer buf) {
+ return wrap(new BinaryTuple(rowLen, buf));
+ }
+
+ /** {@inheritDoc} */
+ @Override
+ public RowWrapper wrap(InternalTuple tuple) {
+ assert rowLen == tuple.elementCount();
+
+ return new BinaryTupleRowWrapper(rowSchema, tuple);
+ }
+ };
+ }
+
+ /**
+ * Provides the ability for a single {@link RowHandler} instance to interact with multiple row implementations.
+ */
+ public abstract static class RowWrapper {
+ abstract int columnsCount();
+
+ abstract RowSchema rowSchema();
+
+ abstract @Nullable Object get(int field);
+
+ abstract void set(int field, Object value);
+
+ abstract ByteBuffer toByteBuffer();
+ }
+
+ /**
+ * Wrapper over an array of objects.
+ */
+ private static class ObjectsArrayRowWrapper extends RowWrapper {
+ private final RowSchema rowSchema;
+ private final Object[] row;
+
+ ObjectsArrayRowWrapper(RowSchema rowSchema, Object[] row) {
+ this.rowSchema = rowSchema;
+ this.row = row;
+ }
+
+ @Override
+ int columnsCount() {
+ return row.length;
+ }
+
+ @Override
+ @Nullable Object get(int field) {
+ return row[field];
+ }
+
+ @Override
+ void set(int field, @Nullable Object value) {
+ row[field] = value;
+ }
+
+ @Override
+ ByteBuffer toByteBuffer() {
+ return RowConverter.toByteBuffer(rowSchema, row);
+ }
+
+ @Override
+ RowSchema rowSchema() {
+ return rowSchema;
+ }
+ }
+
+ /**
+ * Wrapper over an {@link BinaryTuple}.
+ *
+ * <p>Since {@link BinaryTuple binary tuple} is immutable this wrapper doesn't support {@link #set(int, Object)} operation.
+ */
+ private static class BinaryTupleRowWrapper extends RowWrapper {
+ private final RowSchema rowSchema;
+ private final InternalTuple tuple;
+
+ BinaryTupleRowWrapper(RowSchema rowSchema, InternalTuple tuple) {
+ this.rowSchema = rowSchema;
+ this.tuple = tuple;
+ }
+
+ @Override
+ int columnsCount() {
+ return tuple.elementCount();
+ }
+
+ @Override
+ @Nullable Object get(int field) {
+ Object value = rowSchema.value(tuple, field);
+
+ if (value == null) {
+ return null;
+ }
+
+ NativeType nativeType = RowSchemaTypes.toNativeType(rowSchema.fields().get(field));
+
+ return TypeUtils.toInternal(value, Commons.nativeTypeToClass(nativeType));
+ }
+
+ @Override
+ void set(int field, @Nullable Object value) {
+ throw new UnsupportedOperationException();
Review Comment:
how to make sure no one will invoke `set` for row that is wrapper around BinaryTuple?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/RowConverter.java:
##########
@@ -108,34 +121,44 @@ public static <RowT> BinaryTuple toBinaryTuple(
assert rowColumnsCount == binarySchema.elementCount() : "Invalid lookup key.";
- if (IgniteUtils.assertionsEnabled()) {
- for (int i = 0; i < rowColumnsCount; i++) {
- if (handler.get(i, searchRow) == UNSPECIFIED_VALUE_PLACEHOLDER) {
- throw new AssertionError("Invalid lookup key.");
- }
- }
- }
-
BinaryTupleBuilder tupleBuilder = new BinaryTupleBuilder(rowColumnsCount);
- return new BinaryTuple(rowColumnsCount, toByteBuffer(ectx, binarySchema, handler, tupleBuilder, searchRow));
+ return new BinaryTuple(rowColumnsCount, toByteBuffer(binarySchema, handler, tupleBuilder, rowColumnsCount, searchRow));
+ }
+
+ /**
+ * Converts an array of objects into a {@link ByteBuffer} representation of a {@link BinaryTuple binary tuple}. This method uses
+ * {@link RowSchema} to resolve object types.
+ *
+ * @param schema Row schema.
+ * @param values Array of objects to be stored in the buffer.
+ * @return Buffer in binary tuple format.
+ */
+ static ByteBuffer toByteBuffer(RowSchema schema, Object[] values) {
Review Comment:
why do you put this method here rather than to SqlRowHandler?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/RowHandler.java:
##########
@@ -87,5 +105,13 @@ interface RowFactory<RowT> {
* @return Instantiation defined representation.
*/
RowT create(ByteBuffer raw);
+
+ /**
+ * Wraps incoming binary tuple into a instantiation defined representation.
Review Comment:
> into a instantiation defined representation.
not sure I understand this part... Could you reword this please?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/TableRowConverterImpl.java:
##########
@@ -53,30 +59,36 @@ public <RowT> RowT toRow(
RowHandler.RowFactory<RowT> factory,
@Nullable BitSet requiredColumns
) {
- RowHandler<RowT> handler = factory.handler();
+ Row row = schemaRegistry.resolve(binaryRow, schemaDescriptor);
- assert handler == ectx.rowHandler();
+ BinaryTupleBuilder builder = requiredColumns == null
Review Comment:
why don't return BinaryTuple from `allColumnsTuple`/`requiredColumnsTuple`?
--
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