Skip to content
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
49a2350
add the code change to fix the sql query
SaurabhChawla100 Jul 8, 2020
195bc23
handle the scenrios for orc file created by hive data
SaurabhChawla100 Jul 9, 2020
9b51d67
Refactor the unit test as per the review comments
SaurabhChawla100 Jul 10, 2020
b9282d0
Remove drop table from the test as its already handled in withTable
SaurabhChawla100 Jul 11, 2020
ff938a5
Refactor the code change as per the review comments
SaurabhChawla100 Jul 11, 2020
45be048
add the unit test for ORC_VECTORIZED_READER_ENABLED
SaurabhChawla100 Jul 11, 2020
c2ca484
add the code change to handle the schema sent through requestedColumnIds
SaurabhChawla100 Jul 12, 2020
f8f7aff
removed the extra import from OrcFileFormat.scala
SaurabhChawla100 Jul 12, 2020
c2d7a21
refactor the code using seprate varible for sendActualSchema
SaurabhChawla100 Jul 13, 2020
4469e6d
Merge the orcimpl and vectorised test for complete code coverage
SaurabhChawla100 Jul 13, 2020
743ffe3
Merge remote-tracking branch 'os/master' into SPARK-32234
SaurabhChawla100 Jul 13, 2020
9de3516
reafactor the code and add similar logic in file source v2 code path
SaurabhChawla100 Jul 14, 2020
f8ece1f
refactor the code
SaurabhChawla100 Jul 14, 2020
5b8715e
Refactor some method return type and use it the code
SaurabhChawla100 Jul 15, 2020
d0f6b9b
Removed the code of location for create table in unit test
SaurabhChawla100 Jul 15, 2020
c79794f
fix the scala style in the unit test
SaurabhChawla100 Jul 15, 2020
cf68729
created new method for duplicate code
SaurabhChawla100 Jul 15, 2020
6150b08
switch the name of requestedColIds and requestedDataColIds
SaurabhChawla100 Jul 15, 2020
c0f6209
fix the name of the unit test
SaurabhChawla100 Jul 16, 2020
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 @@ -160,12 +160,12 @@ class OrcFileFormat
}

val resultSchema = StructType(requiredSchema.fields ++ partitionSchema.fields)
val actualSchema = StructType(dataSchema.fields ++ partitionSchema.fields)
val sqlConf = sparkSession.sessionState.conf
val enableVectorizedReader = supportBatch(sparkSession, resultSchema)
val capacity = sqlConf.orcVectorizedReaderBatchSize

val resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.MAPRED_INPUT_SCHEMA.setString(hadoopConf, resultSchemaString)
Comment thread
cloud-fan marked this conversation as resolved.
var resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.IS_SCHEMA_EVOLUTION_CASE_SENSITIVE.setBoolean(hadoopConf, sqlConf.caseSensitiveAnalysis)

val broadcastedConf =
Expand All @@ -179,12 +179,17 @@ class OrcFileFormat

val fs = filePath.getFileSystem(conf)
val readerOptions = OrcFile.readerOptions(conf).filesystem(fs)
val requestedColIdsOrEmptyFile =
val (requestedColIdsOrEmptyFile, canPruneCols) =
Utils.tryWithResource(OrcFile.createReader(filePath, readerOptions)) { reader =>
OrcUtils.requestedColumnIds(
isCaseSensitive, dataSchema, requiredSchema, reader, conf)
}

if (!canPruneCols) {

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.

nit: we can simplify the code a bit

val resultSchemaString = if (canPruneCols) {
  OrcUtils.orcTypeDescriptionString(resultSchema)
} else {
  OrcUtils.orcTypeDescriptionString(StructType(dataSchema.fields ++ partitionSchema.fields))
}

Then we don't need to keep the val actualSchema =... and var resultSchemaString =... at the beginning.

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.

done

resultSchemaString = OrcUtils.orcTypeDescriptionString(actualSchema)
Comment thread
cloud-fan marked this conversation as resolved.
Outdated
}
OrcConf.MAPRED_INPUT_SCHEMA.setString(conf, resultSchemaString)

if (requestedColIdsOrEmptyFile.isEmpty) {
Iterator.empty
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,47 +116,53 @@ object OrcUtils extends Logging {
}

/**
* Returns the requested column ids from the given ORC file. Column id can be -1, which means the
* requested column doesn't exist in the ORC file. Returns None if the given ORC file is empty.
* @return Returns the requested column ids from the given ORC file and Boolean flag to use actual

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.

can we update the comment a little bit?

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.

updated the comment

* schema or result schema. Column id can be -1, which means the requested column doesn't
* exist in the ORC file. Returns None if the given ORC file is empty.

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.

Could you add @return for describing what's a return value (Option[Array[Int]], Boolean)?

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.

done

*/
def requestedColumnIds(
isCaseSensitive: Boolean,
dataSchema: StructType,
requiredSchema: StructType,
reader: Reader,
conf: Configuration): Option[Array[Int]] = {
conf: Configuration): (Option[Array[Int]], Boolean) = {

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.

I think the return type should be Option[(Array[Int], Boolean)]

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 did similar change by returning the one attribute in the return type. I was asked to make it two attributes to return from this method
Please find the review comment for that - #29045 (comment)

So I change it after this review comment

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.

how about

def requestedColumnIds(...): Option[(Array[Int], Boolean)]
...
val result = ... { OrcUtils.requestedColumnIds... }
if (result.isEmpty) {
  Iterator.empty
} else {
  val (requestedColIds, canPruneCols) = result.get
  ...
}

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.

done

var canPruneCols = true

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.

do we really need it? We can just use boolean literal in the places that return the value.

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.

Removed it

val orcFieldNames = reader.getSchema.getFieldNames.asScala
Comment thread
cloud-fan marked this conversation as resolved.
if (orcFieldNames.isEmpty) {
// SPARK-8501: Some old empty ORC files always have an empty schema stored in their footer.
None
(None, canPruneCols)
} else {
if (orcFieldNames.forall(_.startsWith("_col"))) {
// This is a ORC file written by Hive, no field names in the physical schema, assume the
// physical schema maps to the data scheme by index.
assert(orcFieldNames.length <= dataSchema.length, "The given data schema " +
s"${dataSchema.catalogString} has less fields than the actual ORC physical schema, " +
"no idea which columns were dropped, fail to read.")
Some(requiredSchema.fieldNames.map { name =>
// for ORC file written by Hive, no field names
// in the physical schema, there is a need to send the
// entire dataSchema instead of required schema
canPruneCols = false
(Some(requiredSchema.fieldNames.map { name =>
val index = dataSchema.fieldIndex(name)
if (index < orcFieldNames.length) {
index
} else {
-1
Comment thread
cloud-fan marked this conversation as resolved.
}
})
}), canPruneCols)
} else {
if (isCaseSensitive) {
Some(requiredSchema.fieldNames.zipWithIndex.map { case (name, idx) =>
(Some(requiredSchema.fieldNames.zipWithIndex.map { case (name, idx) =>
if (orcFieldNames.indexWhere(caseSensitiveResolution(_, name)) != -1) {
idx
} else {
-1
}
})
}), canPruneCols)
} else {
// Do case-insensitive resolution only if in case-insensitive mode
val caseInsensitiveOrcFieldMap = orcFieldNames.groupBy(_.toLowerCase(Locale.ROOT))
Some(requiredSchema.fieldNames.zipWithIndex.map { case (requiredFieldName, idx) =>
(Some(requiredSchema.fieldNames.zipWithIndex.map { case (requiredFieldName, idx) =>
caseInsensitiveOrcFieldMap
.get(requiredFieldName.toLowerCase(Locale.ROOT))
.map { matchedOrcFields =>
Expand All @@ -170,7 +176,7 @@ object OrcUtils extends Logging {
idx
}
}.getOrElse(-1)
})
}), canPruneCols)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ case class OrcPartitionReaderFactory(
readDataSchema: StructType,
partitionSchema: StructType) extends FilePartitionReaderFactory {
private val resultSchema = StructType(readDataSchema.fields ++ partitionSchema.fields)
private val actualSchema = StructType(dataSchema.fields ++ partitionSchema.fields)
private val isCaseSensitive = sqlConf.caseSensitiveAnalysis
private val capacity = sqlConf.orcVectorizedReaderBatchSize

Expand All @@ -66,20 +67,24 @@ case class OrcPartitionReaderFactory(
override def buildReader(file: PartitionedFile): PartitionReader[InternalRow] = {
val conf = broadcastedConf.value.value

val resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.MAPRED_INPUT_SCHEMA.setString(conf, resultSchemaString)
var resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.IS_SCHEMA_EVOLUTION_CASE_SENSITIVE.setBoolean(conf, isCaseSensitive)

val filePath = new Path(new URI(file.filePath))

val fs = filePath.getFileSystem(conf)
val readerOptions = OrcFile.readerOptions(conf).filesystem(fs)
val requestedColIdsOrEmptyFile =
val (requestedColIdsOrEmptyFile, canPruneCols) =
Utils.tryWithResource(OrcFile.createReader(filePath, readerOptions)) { reader =>
OrcUtils.requestedColumnIds(
isCaseSensitive, dataSchema, readDataSchema, reader, conf)
}

if (!canPruneCols) {
resultSchemaString = OrcUtils.orcTypeDescriptionString(actualSchema)
}
OrcConf.MAPRED_INPUT_SCHEMA.setString(conf, resultSchemaString)

if (requestedColIdsOrEmptyFile.isEmpty) {
new EmptyPartitionReader[InternalRow]
} else {
Expand Down Expand Up @@ -112,20 +117,24 @@ case class OrcPartitionReaderFactory(
override def buildColumnarReader(file: PartitionedFile): PartitionReader[ColumnarBatch] = {
val conf = broadcastedConf.value.value

val resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.MAPRED_INPUT_SCHEMA.setString(conf, resultSchemaString)
var resultSchemaString = OrcUtils.orcTypeDescriptionString(resultSchema)
OrcConf.IS_SCHEMA_EVOLUTION_CASE_SENSITIVE.setBoolean(conf, isCaseSensitive)

val filePath = new Path(new URI(file.filePath))

val fs = filePath.getFileSystem(conf)
val readerOptions = OrcFile.readerOptions(conf).filesystem(fs)
val requestedColIdsOrEmptyFile =
val (requestedColIdsOrEmptyFile, canPruneCols) =
Utils.tryWithResource(OrcFile.createReader(filePath, readerOptions)) { reader =>
OrcUtils.requestedColumnIds(
isCaseSensitive, dataSchema, readDataSchema, reader, conf)
}

if (!canPruneCols) {
resultSchemaString = OrcUtils.orcTypeDescriptionString(actualSchema)
}
OrcConf.MAPRED_INPUT_SCHEMA.setString(conf, resultSchemaString)

if (requestedColIdsOrEmptyFile.isEmpty) {
new EmptyPartitionReader
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -288,4 +288,35 @@ class HiveOrcQuerySuite extends OrcQueryTest with TestHiveSingleton {
}
}
}

test("SPARK-32234: orc data created by the hive tables having _col fields name" +
Comment thread
cloud-fan marked this conversation as resolved.
Outdated

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.

we can shorten the test name: SPARK-32234: read ORC table with column names all starting with '_col'

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.

done

" for ORC_IMPLEMENTATION") {
Seq("native", "hive").foreach { orcImpl =>
Seq("false", "true").foreach { vectorized =>
withSQLConf(
SQLConf.ORC_IMPLEMENTATION.key -> orcImpl,
SQLConf.ORC_VECTORIZED_READER_ENABLED.key -> vectorized) {
withTempPath { dir =>

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.

nit: we don't need to provide a custom location. CREATE TABLE without LOCATION clause can also reproduce 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.

Removed it

withTable("test_hive_orc_impl") {
spark.sql(
s"""
| CREATE TABLE test_hive_orc_impl
| (_col1 INT, _col2 STRING, _col3 INT)

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.

can this test reproduce the bug? the table schema matches the physical file schema.

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.

yes this can be reproduce by this also,

But I have attached the date_dim tpcds orc data in the jira itself

| STORED AS ORC LOCATION '$dir'
""".stripMargin)
spark.sql(
"""
| INSERT INTO
| test_hive_orc_impl
| VALUES(9, '12', 2020)
""".stripMargin)

val df = spark.sql("SELECT _col2 FROM test_hive_orc_impl")
checkAnswer(df, Row("12"))
}
}
}
}
}
}
}