-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-37896][SQL] Implement a ConstantColumnVector and improve performance of the hidden file metadata #35068
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
3cc7606
2d8beb8
f5a17de
4ca60d7
a6595f0
f68be18
b50ea9e
cc0eaec
1ae5696
c9a9b7f
11139c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.spark.sql.execution.vectorized; | ||
|
|
||
| import org.apache.spark.sql.types.*; | ||
| import org.apache.spark.sql.vectorized.ColumnVector; | ||
| import org.apache.spark.sql.vectorized.ColumnarArray; | ||
| import org.apache.spark.sql.vectorized.ColumnarMap; | ||
| import org.apache.spark.unsafe.types.UTF8String; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.math.BigInteger; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This order of imports look not following Spark style. Java imports should be before Spark's. |
||
|
|
||
| /** | ||
| * This class adds the constant support to ColumnVector. | ||
| * It supports all the types and contains put APIs, | ||
| * which will put the exact same value to all rows. | ||
| * | ||
| * Capacity: The vector only stores one copy of the data, and acts as an unbounded vector | ||
| * (get from any row will return the same value) | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we write a UT for this new vector?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure - working on it! |
||
| public class ConstantColumnVector extends ColumnVector { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking whether we should extend It seems for partition columns, we are doing copying of same value per row (Parquet and ORC). A future improvement is to use the constant column vector we are introducing here to avoid unnecessary operations. @cloud-fan WDYT?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking to extend |
||
|
|
||
| private byte nullData; | ||
| private byte byteData; | ||
| private short shortData; | ||
| private int intData; | ||
| private long longData; | ||
| private float floatData; | ||
| private double doubleData; | ||
| private byte[] byteArrayData; | ||
| private ConstantColumnVector[] childData; | ||
| private ColumnarArray arrayData; | ||
| private ColumnarMap mapData; | ||
|
|
||
| private int numRows; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wondering what's the point of storing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the only place is
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: make this final |
||
|
|
||
| /** | ||
| * Sets up the data type of this constant column vector. | ||
| * @param type | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: this seems useless. |
||
| */ | ||
| public ConstantColumnVector(int numRows, DataType type) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I actually looked at it as well. Seems there's more code change needed if we want to utilize
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sense. Perhaps we can remove |
||
| super(type); | ||
| this.numRows = numRows; | ||
| if (type instanceof StructType) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we handle
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooops |
||
| StructType st = (StructType) type; | ||
| this.childData = new ConstantColumnVector[st.fields().length]; | ||
| } | ||
|
|
||
| // copy and modify from WritableColumnVector | ||
| // could also putChild by users | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment looks unnecessary. |
||
| if (isArray()) { | ||
| DataType childType; | ||
| if (type instanceof ArrayType) { | ||
| childType = ((ArrayType) type).elementType(); | ||
| } else { | ||
| childType = DataTypes.ByteType; | ||
| } | ||
| this.childData = new ConstantColumnVector[1]; | ||
| this.childData[0] = new ConstantColumnVector(numRows, childType); | ||
| } else if (type instanceof StructType) { | ||
| StructType st = (StructType) type; | ||
| this.childData = new ConstantColumnVector[st.fields().length]; | ||
| for (int i = 0; i < childData.length; ++i) { | ||
| this.childData[i] = new ConstantColumnVector(numRows, st.fields()[i].dataType()); | ||
| } | ||
| } else if (type instanceof MapType) { | ||
| // 0: key, 1: value | ||
| MapType mapType = (MapType) type; | ||
| this.childData = new ConstantColumnVector[2]; | ||
| this.childData[0] = new ConstantColumnVector(numRows, mapType.keyType()); | ||
| this.childData[1] = new ConstantColumnVector(numRows, mapType.valueType()); | ||
| } else if (type instanceof CalendarIntervalType) { | ||
| // 0: Months as Int, 1: Days as Int, 2: Microseconds as Long. | ||
| this.childData = new ConstantColumnVector[3]; | ||
| this.childData[0] = new ConstantColumnVector(numRows, DataTypes.IntegerType); | ||
| this.childData[1] = new ConstantColumnVector(numRows, DataTypes.IntegerType); | ||
| this.childData[2] = new ConstantColumnVector(numRows, DataTypes.LongType); | ||
| } else { | ||
| this.childData = null; | ||
| } | ||
| } | ||
|
|
||
| protected boolean isArray() { | ||
| return type instanceof ArrayType || type instanceof BinaryType || type instanceof StringType || | ||
| DecimalType.isByteArrayDecimalType(type); | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| byteArrayData = null; | ||
| childData = null; | ||
|
sunchao marked this conversation as resolved.
|
||
| arrayData = null; | ||
| mapData = null; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean hasNull() { | ||
| return nullData == 1; | ||
| } | ||
|
|
||
| @Override | ||
| public int numNulls() { | ||
| return numRows; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be 0 if |
||
| } | ||
|
|
||
| @Override | ||
| public boolean isNullAt(int rowId) { | ||
| return nullData == 1; | ||
| } | ||
|
|
||
| public void putNull() { | ||
| nullData = (byte) 1; | ||
| } | ||
|
|
||
| public void putNotNull() { | ||
| nullData = (byte) 0; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean getBoolean(int rowId) { | ||
| return byteData == 1; | ||
| } | ||
|
|
||
| public void putBoolean(boolean value) { | ||
| byteData = (byte) ((value) ? 1 : 0); | ||
| } | ||
|
|
||
| @Override | ||
| public byte getByte(int rowId) { | ||
| return byteData; | ||
| } | ||
|
|
||
| public void putByte(byte value) { | ||
| byteData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public short getShort(int rowId) { | ||
| return shortData; | ||
| } | ||
|
|
||
| public void putShort(short value) { | ||
| shortData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public int getInt(int rowId) { | ||
| return intData; | ||
| } | ||
|
|
||
| public void putInt(int value) { | ||
| intData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public long getLong(int rowId) { | ||
| return longData; | ||
| } | ||
|
|
||
| public void putLong(long value) { | ||
| longData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public float getFloat(int rowId) { | ||
| return floatData; | ||
| } | ||
|
|
||
| public void putFloat(float value) { | ||
| floatData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public double getDouble(int rowId) { | ||
| return doubleData; | ||
| } | ||
|
|
||
| public void putDouble(double value) { | ||
| doubleData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public ColumnarArray getArray(int rowId) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this can work properly. Looking at |
||
| return arrayData; | ||
| } | ||
|
|
||
| public void putArray(ColumnarArray value) { | ||
| arrayData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public ColumnarMap getMap(int ordinal) { | ||
| return mapData; | ||
| } | ||
|
|
||
| public void putMap(ColumnarMap value) { | ||
| mapData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public Decimal getDecimal(int rowId, int precision, int scale) { | ||
| // copy and modify from WritableColumnVector | ||
| if (precision <= Decimal.MAX_INT_DIGITS()) { | ||
| return Decimal.createUnsafe(getInt(rowId), precision, scale); | ||
| } else if (precision <= Decimal.MAX_LONG_DIGITS()) { | ||
| return Decimal.createUnsafe(getLong(rowId), precision, scale); | ||
| } else { | ||
| byte[] bytes = getBinary(rowId); | ||
| BigInteger bigInteger = new BigInteger(bytes); | ||
| BigDecimal javaDecimal = new BigDecimal(bigInteger, scale); | ||
| return Decimal.apply(javaDecimal, precision, scale); | ||
| } | ||
| } | ||
|
|
||
| public void putDecimal(Decimal value, int precision) { | ||
| // copy and modify from WritableColumnVector | ||
| if (precision <= Decimal.MAX_INT_DIGITS()) { | ||
| putInt((int) value.toUnscaledLong()); | ||
| } else if (precision <= Decimal.MAX_LONG_DIGITS()) { | ||
| putLong(value.toUnscaledLong()); | ||
| } else { | ||
| BigInteger bigInteger = value.toJavaBigDecimal().unscaledValue(); | ||
| putByteArray(bigInteger.toByteArray()); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public UTF8String getUTF8String(int rowId) { | ||
| return UTF8String.fromBytes(byteArrayData); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we can store a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. make sense, done! |
||
| } | ||
|
|
||
| public void putUtf8String(UTF8String value) { | ||
| putByteArray(value.getBytes()); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe add
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for the suggestion - just wanna put some minimum supports in this PR (implement all necessary APIs extending from |
||
| private void putByteArray(byte[] value) { | ||
| byteArrayData = value; | ||
| } | ||
|
|
||
| @Override | ||
| public byte[] getBinary(int rowId) { | ||
| return byteArrayData; | ||
| } | ||
|
|
||
| public void putBinary(byte[] value) { | ||
| putByteArray(value); | ||
| } | ||
|
|
||
| @Override | ||
| public ColumnVector getChild(int ordinal) { | ||
|
Yaohua628 marked this conversation as resolved.
|
||
| return childData[ordinal]; | ||
| } | ||
|
|
||
| public void putChild(int ordinal, ConstantColumnVector value) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. I'm also in favor of using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok sure, was thinking of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For public api, it's better to add a comment.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not anywhere, for now (just like many other set methods: |
||
| childData[ordinal] = value; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ import org.apache.spark.sql.catalyst.InternalRow | |
| import org.apache.spark.sql.catalyst.expressions.{AttributeReference, GenericInternalRow, JoinedRow, UnsafeProjection, UnsafeRow} | ||
| import org.apache.spark.sql.errors.QueryExecutionErrors | ||
| import org.apache.spark.sql.execution.datasources.FileFormat._ | ||
| import org.apache.spark.sql.execution.vectorized.{OnHeapColumnVector, WritableColumnVector} | ||
| import org.apache.spark.sql.execution.vectorized.ConstantColumnVector | ||
| import org.apache.spark.sql.types.{LongType, StringType, StructType} | ||
| import org.apache.spark.sql.vectorized.ColumnarBatch | ||
| import org.apache.spark.unsafe.types.UTF8String | ||
|
|
@@ -135,58 +135,44 @@ class FileScanRDD( | |
| * Only update metadata row when `currentFile` is changed. | ||
| */ | ||
| private def updateMetadataRow(): Unit = { | ||
| if (metadataColumns.nonEmpty && currentFile != null) { | ||
| val path = new Path(currentFile.filePath) | ||
| metadataColumns.zipWithIndex.foreach { case (attr, i) => | ||
| attr.name match { | ||
| case FILE_PATH => metadataRow.update(i, UTF8String.fromString(path.toString)) | ||
| case FILE_NAME => metadataRow.update(i, UTF8String.fromString(path.getName)) | ||
| case FILE_SIZE => metadataRow.update(i, currentFile.fileSize) | ||
| case FILE_MODIFICATION_TIME => | ||
| // the modificationTime from the file is in millisecond, | ||
| // while internally, the TimestampType is stored in microsecond | ||
| metadataRow.update(i, currentFile.modificationTime * 1000L) | ||
| } | ||
| if (metadataColumns.isEmpty || currentFile == null) return | ||
| val path = new Path(currentFile.filePath) | ||
| metadataColumns.zipWithIndex.foreach { case (attr, i) => | ||
| attr.name match { | ||
| case FILE_PATH => metadataRow.update(i, UTF8String.fromString(path.toString)) | ||
| case FILE_NAME => metadataRow.update(i, UTF8String.fromString(path.getName)) | ||
| case FILE_SIZE => metadataRow.update(i, currentFile.fileSize) | ||
| case FILE_MODIFICATION_TIME => | ||
| // the modificationTime from the file is in millisecond, | ||
| // while internally, the TimestampType is stored in microsecond | ||
| metadataRow.update(i, currentFile.modificationTime * 1000L) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: unnecessary change? didn't feel the readability improved much after negating the if condition. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create a writable column vector containing all required metadata columns | ||
| * Create a constant column vector containing all required metadata columns | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: shouldn't be: |
||
| */ | ||
| private def createMetadataColumnVector(c: ColumnarBatch): Array[WritableColumnVector] = { | ||
| private def createMetadataColumnVector(c: ColumnarBatch): Array[ConstantColumnVector] = { | ||
| val path = new Path(currentFile.filePath) | ||
| val filePathBytes = path.toString.getBytes | ||
| val fileNameBytes = path.getName.getBytes | ||
| var rowId = 0 | ||
| metadataColumns.map(_.name).map { | ||
| case FILE_PATH => | ||
| val columnVector = new OnHeapColumnVector(c.numRows(), StringType) | ||
| rowId = 0 | ||
| // use a tight-loop for better performance | ||
| while (rowId < c.numRows()) { | ||
| columnVector.putByteArray(rowId, filePathBytes) | ||
| rowId += 1 | ||
| } | ||
| val columnVector = new ConstantColumnVector(c.numRows(), StringType) | ||
| columnVector.putUtf8String(UTF8String.fromString(path.toString)) | ||
| columnVector | ||
| case FILE_NAME => | ||
| val columnVector = new OnHeapColumnVector(c.numRows(), StringType) | ||
| rowId = 0 | ||
| // use a tight-loop for better performance | ||
| while (rowId < c.numRows()) { | ||
| columnVector.putByteArray(rowId, fileNameBytes) | ||
| rowId += 1 | ||
| } | ||
| val columnVector = new ConstantColumnVector(c.numRows(), StringType) | ||
| columnVector.putUtf8String(UTF8String.fromString(path.getName)) | ||
| columnVector | ||
| case FILE_SIZE => | ||
| val columnVector = new OnHeapColumnVector(c.numRows(), LongType) | ||
| columnVector.putLongs(0, c.numRows(), currentFile.fileSize) | ||
| val columnVector = new ConstantColumnVector(c.numRows(), LongType) | ||
| columnVector.putLong(currentFile.fileSize) | ||
| columnVector | ||
| case FILE_MODIFICATION_TIME => | ||
| val columnVector = new OnHeapColumnVector(c.numRows(), LongType) | ||
| val columnVector = new ConstantColumnVector(c.numRows(), LongType) | ||
| // the modificationTime from the file is in millisecond, | ||
| // while internally, the TimestampType is stored in microsecond | ||
| columnVector.putLongs(0, c.numRows(), currentFile.modificationTime * 1000L) | ||
| columnVector.putLong(currentFile.modificationTime * 1000L) | ||
| columnVector | ||
| }.toArray | ||
| } | ||
|
|
@@ -198,10 +184,9 @@ class FileScanRDD( | |
| private def addMetadataColumnsIfNeeded(nextElement: Object): Object = { | ||
| if (metadataColumns.nonEmpty) { | ||
| nextElement match { | ||
| case c: ColumnarBatch => | ||
| new ColumnarBatch( | ||
| Array.tabulate(c.numCols())(c.column) ++ createMetadataColumnVector(c), | ||
| c.numRows()) | ||
| case c: ColumnarBatch => new ColumnarBatch( | ||
| Array.tabulate(c.numCols())(c.column) ++ createMetadataColumnVector(c), | ||
| c.numRows()) | ||
| case u: UnsafeRow => projection.apply(new JoinedRow(u, metadataRow)) | ||
| case i: InternalRow => new JoinedRow(i, metadataRow) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the Apache license header similar to other files.