Skip to content

[SPARK-21912][SQL] ORC/Parquet table should not create invalid column names #19124

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.

Sign up for GitHub

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

Closed
dongjoon-hyun wants to merge 12 commits into from
Closed

[SPARK-21912][SQL] ORC/Parquet table should not create invalid column names #19124

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
808dfe0
[SPARK-21912][SQL] Creating ORC datasource table should check invalid…
dongjoon-hyun Sep 4, 2017
a738943
Address comments.
dongjoon-hyun Sep 5, 2017
cd539fe
Address comments.
dongjoon-hyun Sep 5, 2017
66aff54
Add a newline at the end of the file.
dongjoon-hyun Sep 5, 2017
aa78eaf
fix
dongjoon-hyun Sep 5, 2017
0bf3b43
Update answer file to show the result.
dongjoon-hyun Sep 5, 2017
79929e9
Replace parquet with json.
dongjoon-hyun Sep 5, 2017
368b242
Address comments.
dongjoon-hyun Sep 5, 2017
c70c03c
Use DataSourceStategy.
dongjoon-hyun Sep 5, 2017
8ee87dd
Move to DDLUtils.
dongjoon-hyun Sep 5, 2017
c6e9ab6
Add ALTER TABLE and address comments.
dongjoon-hyun Sep 6, 2017
46847f8
Address comments.
dongjoon-hyun Sep 6, 2017
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
19 changes: 17 additions & 2 deletions sql/hive/src/main/scala/org/apache/spark/sql/hive/orc/OrcFileFormat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,15 @@ import org.apache.hadoop.io.{NullWritable, Writable}
import org.apache.hadoop.mapred.{JobConf, OutputFormat => MapRedOutputFormat, RecordWriter, Reporter}
import org.apache.hadoop.mapreduce._
import org.apache.hadoop.mapreduce.lib.input.{FileInputFormat, FileSplit}
import org.apache.orc.TypeDescription

import org.apache.spark.TaskContext
import org.apache.spark.sql.{Row, SparkSession}
import org.apache.spark.sql._
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.execution.datasources._
import org.apache.spark.sql.hive.{HiveInspectors, HiveShim}
import org.apache.spark.sql.sources.{Filter, _}
import org.apache.spark.sql.sources._
import org.apache.spark.sql.types.StructType
import org.apache.spark.util.SerializableConfiguration

Expand Down Expand Up @@ -83,6 +84,8 @@ class OrcFileFormat extends FileFormat with DataSourceRegister with Serializable
classOf[MapRedOutputFormat[_, _]])
}

dataSchema.map(_.name).foreach(checkFieldName)

new OutputWriterFactory {
override def newInstance(
path: String,
Expand Down Expand Up @@ -169,6 +172,18 @@ class OrcFileFormat extends FileFormat with DataSourceRegister with Serializable
}
}
}

private def checkFieldName(name: String): Unit = {
try {
TypeDescription.fromString(s"struct<$name:int>")
} catch {
case _: IllegalArgumentException =>
throw new AnalysisException(
s"""Attribute name "$name" contains invalid character(s).
Copy link
Member

@gatorsmile gatorsmile Sep 5, 2017

Choose a reason for hiding this comment

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

Nit: Attribute-> Column

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017

Choose a reason for hiding this comment

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

Thank you for review. Sure.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017

Choose a reason for hiding this comment

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

I agree with you that column is more accurate here. Previously, I borrowed this from ParquetSchemaConverter

https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaConverter.scala#L565-L572

|Please use alias to rename it.
""".stripMargin.split("\n").mkString(" ").trim)
}
}
}

private[orc] class OrcSerializer(dataSchema: StructType, conf: Configuration)
Expand Down
11 changes: 11 additions & 0 deletions sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2000,4 +2000,15 @@ class SQLQuerySuite extends QueryTest with SQLTestUtils with TestHiveSingleton {
assert(setOfPath.size() == pathSizeToDeleteOnExit)
}
}

test("SPARK-21912 Creating ORC datasource table should check invalid column names") {
withTable("orc1") {
Seq(" ", "?", ",", ";", "{", "}", "(", ")", "\n", "\t", "=").foreach { name =>
val m = intercept[AnalysisException] {
sql(s"CREATE TABLE orc1 USING ORC AS SELECT 1 `column$name`")
Copy link
Member

@gatorsmile gatorsmile Sep 5, 2017

Choose a reason for hiding this comment

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

This is CTAS. How about CREATE TABLE?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yep. I'll check the code path, too.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017

Choose a reason for hiding this comment

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

It seems to be the same situation with Parquet. CREATE TABLE passes but SELECT raises exceptions.

scala> sql("CREATE TABLE parquet1(`a b` int) using parquet")
res1: org.apache.spark.sql.DataFrame = []

scala> sql("select * from parquet1").show
org.apache.spark.sql.AnalysisException: Attribute name "a b" contains invalid character(s) among " ,;{}()\n\t=". Please use alias to rename it.;

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017

Choose a reason for hiding this comment

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

Do we need to add Datasource specific operation on createDataSourceTables for Parquet and ORC?

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@gatorsmile . I tried the following in CreateDataSourceTableCommand. We can add a check for ParquetFileFormat, but not for OrcFileFormat. Should I change the PR title and scope instead?

    table.provider.get.toLowerCase match {
      case "parquet" =>
        dataSource.schema.map(_.name).foreach(
          org.apache.spark.sql.execution.datasources.parquet.ParquetSchemaConverter.checkFieldName)
      case "orc" =>
        dataSource.schema.map(_.name).foreach(
          org.apache.spark.sql.hive.OrcRelation.checkFieldName)
    }

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Sep 5, 2017

Choose a reason for hiding this comment

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

I'll try in another way.

}.getMessage
assert(m.contains(s"contains invalid character(s)"))
}
}
}
}