Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,10 @@ public int size() {

@Override
public <T> T get(int pos, Class<T> javaClass) {
if (struct == null) {
return null;
Comment thread
openinx marked this conversation as resolved.
}

int structPos = positionMap[pos];

if (nestedProjections[pos] != null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
/*
* 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.iceberg.flink.data;

import org.apache.flink.table.data.ArrayData;
import org.apache.flink.table.data.DecimalData;
import org.apache.flink.table.data.MapData;
import org.apache.flink.table.data.RawValueData;
import org.apache.flink.table.data.RowData;
import org.apache.flink.table.data.StringData;
import org.apache.flink.table.data.TimestampData;
import org.apache.flink.table.types.logical.RowType;
import org.apache.flink.types.RowKind;
import org.apache.iceberg.Schema;
import org.apache.iceberg.flink.FlinkSchemaUtil;
import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.types.Types;

public class RowDataProjection implements RowData {
Comment thread
Reo-LEI marked this conversation as resolved.

private final RowData.FieldGetter[] getters;
private RowData rowData;

public static RowDataProjection create(Schema schema, Schema projectSchema) {
return new RowDataProjection(FlinkSchemaUtil.convert(schema), schema.asStruct(), projectSchema.asStruct());
}

private RowDataProjection(RowType rowType, Types.StructType rowStruct, Types.StructType projectType) {
this.getters = new RowData.FieldGetter[projectType.fields().size()];
for (int i = 0; i < getters.length; i++) {
getters[i] = createFieldGetter(rowType, rowStruct, projectType.fields().get(i));
}
}

private static RowData.FieldGetter createFieldGetter(RowType rowType,
Types.StructType rowStruct,
Types.NestedField projectField) {
for (int i = 0; i < rowStruct.fields().size(); i++) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this loop find essentially results in n^2 complexity. We can use this API from StructType.

    public NestedField field(int id) 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we not only need to found the row field which field id equal to project field id, but also need to know the position of the match field. Even if we can get the match row field by StructType.field(int id), we also need to traverse the rowStruct to found out the field position again.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Can we iterate through the schema once and set up the mapping btw field id and position id? I have a little performance concern of n^2 complexity for table with a lot of columns (like thousands or more).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. There are tables with very high cardinality where this will potentially have a real performance impact. This tends to be especially true for base tables (raw ingested data events from clients etc), which often have very wide schemas and is also an area where Flink is pretty commonly used.

Anything that can be done to reduce this overhead would be great.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a great idea, let's do this~

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a little performance concern of n^2 complexity for table with a lot of columns (like thousands or more)

I'm fine with either. Because the complexity is actually n*m, let's say the n is the table's field number and m is the projection fields number. If both @stevenzwu and @kbendick think it's necessary to do, I'm okay with it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a little performance concern of n^2 complexity for table with a lot of columns (like thousands or more)

I'm fine with either. Because the complexity is actually n*m, let's say the n is the table's field number and m is the projection fields number. If both @stevenzwu and @kbendick think it's necessary to do, I'm okay with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I construct a fieldIdToPosition map and use StructType.field(int id) to find the row field. Now the complexity reduce to n, I think the performance will not be a problem.

Types.NestedField rowField = rowStruct.fields().get(i);
if (rowField.fieldId() == projectField.fieldId()) {
Preconditions.checkArgument(rowField.type().typeId() == projectField.type().typeId(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can be simplified by the following lines as the Preconditions.checkArgument can format the error message directly.

        Preconditions.checkArgument(rowField.type().typeId() == projectField.type().typeId(),
            "Different iceberg type between row field <%s> and project field <%s>",
            rowField, projectField);

String.format("Different iceberg type between row field <%s> and project field <%s>",
rowField, projectField));

switch (projectField.type().typeId()) {
case STRUCT:
RowType nestedRowType = (RowType) rowType.getTypeAt(i);
int rowPos = i;
return row -> {
RowData nestedRow = row.isNullAt(rowPos) ? null : row.getRow(rowPos, nestedRowType.getFieldCount());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: If the nestedRow is null, do we still need to traverse the nested fields by using the RowDataProjection#project ? I think we can just return the null for the projection value ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a small patch for this:

diff --git a/flink/src/main/java/org/apache/iceberg/flink/data/RowDataProjection.java b/flink/src/main/java/org/apache/iceberg/flink/data/RowDataProjection.java
index 9d1e8ea67..25a5b3ab3 100644
--- a/flink/src/main/java/org/apache/iceberg/flink/data/RowDataProjection.java
+++ b/flink/src/main/java/org/apache/iceberg/flink/data/RowDataProjection.java
@@ -45,7 +45,11 @@ public class RowDataProjection implements RowData {
    * @return a wrapper to project rows
    */
   public static RowDataProjection create(Schema schema, Schema projectedSchema) {
-    return new RowDataProjection(FlinkSchemaUtil.convert(schema), schema.asStruct(), projectedSchema.asStruct());
+    return RowDataProjection.create(FlinkSchemaUtil.convert(schema), schema.asStruct(), projectedSchema.asStruct());
+  }
+
+  public static RowDataProjection create(RowType rowType, Types.StructType schema, Types.StructType projectedSchema) {
+    return new RowDataProjection(rowType, schema, projectedSchema);
   }
 
   private final RowData.FieldGetter[] getters;
@@ -73,9 +77,14 @@ public class RowDataProjection implements RowData {
             RowType nestedRowType = (RowType) rowType.getTypeAt(i);
             int rowPos = i;
             return row -> {
-              RowData nestedRow = row.isNullAt(rowPos) ? null : row.getRow(rowPos, nestedRowType.getFieldCount());
-              return new RowDataProjection(nestedRowType, rowField.type().asStructType(),
-                  projectField.type().asStructType()).wrap(nestedRow);
+              if (row.isNullAt(rowPos)) {
+                return null;
+              } else {
+                RowData nestedRow = row.getRow(rowPos, nestedRowType.getFieldCount());
+                return RowDataProjection
+                    .create(nestedRowType, rowField.type().asStructType(), projectField.type().asStructType())
+                    .wrap(nestedRow);
+              }
             };
 
           case MAP:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could not return null when the nestedRow is null. Because StructProjection will still project the nested struct even if the nested struct is null. If we return null here, the unittest will fail, because the expected record is not null but actual row data is null.

Assert.assertTrue("expected Record and actual RowData should be both null or not null",

return new RowDataProjection(nestedRowType, rowField.type().asStructType(),
projectField.type().asStructType()).wrap(nestedRow);
};

case MAP:
Types.MapType projectedMap = projectField.type().asMapType();
Types.MapType originalMap = rowField.type().asMapType();

boolean keyProjectable = !projectedMap.keyType().isNestedType() ||
projectedMap.keyType().equals(originalMap.keyType());
boolean valueProjectable = !projectedMap.valueType().isNestedType() ||
projectedMap.valueType().equals(originalMap.valueType());
Preconditions.checkArgument(keyProjectable && valueProjectable,
"Cannot project a partial map key or value RowData. Trying to project %s out of %s",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should say Cannot project a partial map key or value with non-primitive type, Trying .., the assert failure does not mean it's necessary to be a RowData, it can be other data types such as list or map etc.

projectField, rowField);

return RowData.createFieldGetter(rowType.getTypeAt(i), i);

case LIST:
Types.ListType projectedList = projectField.type().asListType();
Types.ListType originalList = rowField.type().asListType();

boolean elementProjectable = !projectedList.elementType().isNestedType() ||
projectedList.elementType().equals(originalList.elementType());
Preconditions.checkArgument(elementProjectable,
"Cannot project a partial list element RowData. Trying to project %s out of %s",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note below about this exception message.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I trying to keep this message same as StructLikeProjection. I feel this msg is ok, What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

projectField, rowField);

return RowData.createFieldGetter(rowType.getTypeAt(i), i);

default:
return RowData.createFieldGetter(rowType.getTypeAt(i), i);
}
}
}
throw new IllegalArgumentException(String.format("Cannot find field %s in %s", projectField, rowStruct));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we need a more clear message for this exception: Cannot locate the project field <%s> in the iceberg struct <%s>

}

public RowData wrap(RowData row) {
Comment thread
openinx marked this conversation as resolved.
this.rowData = row;
return this;
}

public Object getValue(int pos) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this can be a private method, right ?

return getters[pos].getFieldOrNull(rowData);
}

@Override
public int getArity() {
return getters.length;
}

@Override
public RowKind getRowKind() {
return rowData.getRowKind();
}

@Override
public void setRowKind(RowKind kind) {
throw new UnsupportedOperationException("Cannot set row kind in the RowDataProjection");
}

@Override
public boolean isNullAt(int pos) {
return rowData == null || getValue(pos) == null;
}

@Override
public boolean getBoolean(int pos) {
return (boolean) getValue(pos);
}

@Override
public byte getByte(int pos) {
return (byte) getValue(pos);
}

@Override
public short getShort(int pos) {
return (short) getValue(pos);
}

@Override
public int getInt(int pos) {
return (int) getValue(pos);
}

@Override
public long getLong(int pos) {
return (long) getValue(pos);
}

@Override
public float getFloat(int pos) {
return (float) getValue(pos);
}

@Override
public double getDouble(int pos) {
return (double) getValue(pos);
}

@Override
public StringData getString(int pos) {
return (StringData) getValue(pos);
}

@Override
public DecimalData getDecimal(int pos, int precision, int scale) {
return (DecimalData) getValue(pos);
}

@Override
public TimestampData getTimestamp(int pos, int precision) {
// return getValue(pos, TimestampData.class);
Comment thread
Reo-LEI marked this conversation as resolved.
Outdated
return (TimestampData) getValue(pos);
}

@Override
@SuppressWarnings("unchecked")
public <T> RawValueData<T> getRawValue(int pos) {
// return getValue(pos, RawValueData.class);
return (RawValueData<T>) getValue(pos);
}

@Override
public byte[] getBinary(int pos) {
// return getValue(pos, byte[].class);
return (byte[]) getValue(pos);
}

@Override
public ArrayData getArray(int pos) {
// return getValue(pos, ArrayData.class);
return (ArrayData) getValue(pos);
}

@Override
public MapData getMap(int pos) {
// return getValue(pos, MapData.class);
return (MapData) getValue(pos);
}

@Override
public RowData getRow(int pos, int numFields) {
// return getValue(pos, RowData.class);
return (RowData) getValue(pos);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.iceberg.flink.data.FlinkAvroReader;
import org.apache.iceberg.flink.data.FlinkOrcReader;
import org.apache.iceberg.flink.data.FlinkParquetReaders;
import org.apache.iceberg.flink.data.RowDataProjection;
import org.apache.iceberg.flink.data.RowDataUtil;
import org.apache.iceberg.io.CloseableIterable;
import org.apache.iceberg.io.CloseableIterator;
Expand Down Expand Up @@ -70,9 +71,17 @@ public CloseableIterator<RowData> open(FileScanTask task, InputFilesDecryptor in
PartitionUtil.constantsMap(task, RowDataUtil::convertConstant);

FlinkDeleteFilter deletes = new FlinkDeleteFilter(task, tableSchema, projectedSchema, inputFilesDecryptor);
return deletes
.filter(newIterable(task, deletes.requiredSchema(), idToConstant, inputFilesDecryptor))
.iterator();
CloseableIterable<RowData> iterable = deletes.filter(
newIterable(task, deletes.requiredSchema(), idToConstant, inputFilesDecryptor)
);

// Project the RowData to remove the extra meta columns.
if (!projectedSchema.sameSchema(deletes.requiredSchema())) {
Comment thread
openinx marked this conversation as resolved.
RowDataProjection rowDataProjection = RowDataProjection.create(deletes.requiredSchema(), projectedSchema);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the RowDataProjection#create does a FlinkSchemaUtil.convert(schema) for the required schema to project, and I believe the FlinkDeleteFilter also did the same thing inside. I think we can reuse the converted flink row type between them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Now I get the row type from FlinkDeleteFilter and pass it to RowDataProjection.

iterable = CloseableIterable.transform(iterable, rowDataProjection::wrap);
}

return iterable.iterator();
}

private CloseableIterable<RowData> newIterable(
Expand Down
Loading