[SPARK-26551][SQL] Fix schema pruning error when selecting one complex field and having is not null predicate on another one

[viirya]

What changes were proposed in this pull request?

Schema pruning has errors when selecting one complex field and having is not null predicate on another one:

valquery= sql("select * from contacts")
.where("name.middle is not null")
.select(
"id",
"name.first",
"name.middle",
"name.last"
)
.where("last = 'Jones'")
.select(count("id"))

java.lang.IllegalArgumentException: middle does not exist. Available: last [info] at org.apache.spark.sql.types.StructType.$anonfun$fieldIndex$1(StructType.scala:303) [info] at scala.collection.immutable.Map$Map1.getOrElse(Map.scala:119) [info] at org.apache.spark.sql.types.StructType.fieldIndex(StructType.scala:302) [info] at org.apache.spark.sql.execution.ProjectionOverSchema.$anonfun$getProjection$6(ProjectionOverSchema.scala:58) [info] at scala.Option.map(Option.scala:163) [info] at org.apache.spark.sql.execution.ProjectionOverSchema.getProjection(ProjectionOverSchema.scala:56) [info] at org.apache.spark.sql.execution.ProjectionOverSchema.unapply(ProjectionOverSchema.scala:32) [info] at org.apache.spark.sql.execution.datasources.parquet.ParquetSchemaPruning$$anonfun$$nestedInanonfun$buildNewProjection$1$1.applyOrElse(Parque
tSchemaPruning.scala:153) 

How was this patch tested?

Added tests.

[viirya]

cc @dbtsai@cloud-fan@dongjoon-hyun

[SparkQA]

Test build #100820 has finished for PR 23474 at commit 4f5a91a.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please.

[SparkQA]

Test build #100825 has finished for PR 23474 at commit 4f5a91a.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please...

[SparkQA]

Test build #100829 has finished for PR 23474 at commit 4f5a91a.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

Thank you for pinging me, @viirya !

[SparkQA]

Test build #100861 has finished for PR 23474 at commit 6dbd753.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[HyukjinKwon]

retest this please

[SparkQA]

Test build #100864 has finished for PR 23474 at commit 6dbd753.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

retest this please.

[SparkQA]

Test build #100869 has finished for PR 23474 at commit 6dbd753.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dbtsai]

When we read name.first and check if name is not null, we are marking name as contentAccessed = false to avoid reading the entire name column.

Now, the issue is when we read name.first and check if name.middle is not null, we are still marking name.middle as contentAccessed = false resulting java.lang.IllegalArgumentException: middle does not exist..

To fix the root cause, and avoid the misunderstanding of the meaning of contentAccessed, we might mark contentAccessed = true in the second case; thus, this change here will not be required.

What do you think?

Thanks.

[viirya]

I think It is harder to mark name.middle as contentAccessed = false. It is true we can check all field accesses and see if middle is not accessed by others. But I feel it is more difficult to do that and current fix is simpler.

[dbtsai]

If it's really difficult to mark name.middle as contentAccessed = true in this case (which I feel is a less hacky solution), can we reformat the code with the following with documentation?

!rootFields.exists { root =>
root.field.name == opt.field.name && {
// If the merged field type of root and opt field is different from opt field type,// we will keep it.// For example, when root field type is `struct<name:struct<last:string>>`,// and opt field type is `struct<name:struct<middle:string>>`, the merged field type will be// `struct<name:struct<last:string,middle:string>>`. Since the merged one contains more// nested fields than opt field type, we have to keep it.valrootFieldType=StructType(Array(root.field))
valoptFieldType=StructType(Array(opt.field))
valmerged= optFieldType.merge(rootFieldType)
merged.sameType(optFieldType)
}

Add @hvanhovell@gatorsmile for more input.

Thanks!

[dongjoon-hyun]

+1 for @dbtsai 's refactored code.

[dbtsai]

Can we move optFieldType right after val rootFieldType = StructType(Array(root.field))? Thanks!

[viirya]

Do you mean moving it inside the exists call?

[viirya]

I make it out of exist call so it can be reused, isn't? Moving it to after rootFieldType is for readability?

[dbtsai]

It's not very expensive, and we only need to compute it when root.field.name == opt.field.name. As a result, I feel moving it right after val rootFieldType will be more readable.

 root.field.name == opt.field.name && {
valrootFieldType=StructType(Array(root.field))
valoptFieldType=StructType(Array(opt.field))
valmerged= optFieldType.merge(rootFieldType)
merged.sameType(optFieldType)
}

[viirya]

Ok. I see. Let me move it. Thanks.

[dbtsai]

LGTM. Just one minor comment. Thanks!

[SparkQA]

Test build #101024 has finished for PR 23474 at commit ff1cc85.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #101075 has finished for PR 23474 at commit ff2fa67.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dbtsai]

LGTM. Merged into master and 2.4 branch. Thanks!