-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-12139] [SQL] REGEX Column Specification #18023
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 1 commit
af55afd
6f9bdb0
43beb07
7699e87
6e37517
bee07cd
d5e450a
979bfb6
612bedf
48c54aa
0284d01
129243a
5df5494
779724d
a27023c
e8d4054
201f4d6
a0e3773
6b091dc
537b3bc
da60368
9c582eb
79e58f0
616b726
f98207b
321211d
4e36ed9
04b62c6
2d0dd1c
448c3e2
2ef2c14
d65c462
65e5eec
65886cd
ca89a4a
d3eed1a
956b849
8adad7c
56e2b83
d613ff9
f5104e4
a5f9c44
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 |
|---|---|---|
|
|
@@ -83,6 +83,33 @@ case class UnresolvedTableValuedFunction( | |
| override lazy val resolved = false | ||
| } | ||
|
|
||
| /** | ||
| * Represents all of the input attributes to a given relational operator, for example in | ||
| * "SELECT `(id)?+.+` FROM ...". | ||
| * | ||
| * @param table an optional table that should be the target of the expansion. If omitted all | ||
| * tables' columns are produced. | ||
| */ | ||
| case class UnresolvedRegex(expr: String, table: Option[String]) extends Star with Unevaluable { | ||
| override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = { | ||
| val expandedAttributes: Seq[Attribute] = table match { | ||
| // If there is no table specified, use all input attributes that match expr | ||
| case None => input.output.filter(_.name.matches(expr)) | ||
| // If there is a table, pick out attributes that are part of this table that match expr | ||
|
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. shall we consider "struct expansion" like what we did in
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. see the table argument of UnresolvedStart
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. shall we support
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. for this diff, we support column only. We could consider it in the future.
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. does hive support it?
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. Hive supper regex column specification, see https://cwiki.apache.org/confluence/display/Hive/LanguageManual+Select. Hive does not support record.
col_name data_type commenta int None
FAILED: RuntimeException cannot find field (c)?+.+(lowercase form: (c)?+.+) in [c, d] |
||
| case Some(t) => input.output.filter(_.qualifier.filter(resolver(_, t)).nonEmpty) | ||
|
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.
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. updated. |
||
| .filter(_.name.matches(expr)) | ||
| } | ||
|
|
||
| expandedAttributes.zip(input.output).map { | ||
|
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. An
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. you are right. we dont need it any more. removed. |
||
| case (n: NamedExpression, _) => n | ||
| case (e, originalAttribute) => | ||
| Alias(e, originalAttribute.name)() | ||
| } | ||
| } | ||
|
|
||
| override def toString: String = table.map(_ + ".").getOrElse("") + expr | ||
| } | ||
|
|
||
| /** | ||
| * Holds the name of an attribute that has yet to be resolved. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,10 +23,9 @@ import javax.xml.bind.DatatypeConverter | |
|
|
||
| import scala.collection.JavaConverters._ | ||
| import scala.collection.mutable.ArrayBuffer | ||
|
|
||
| import org.antlr.v4.runtime.{ParserRuleContext, Token} | ||
| import org.antlr.v4.runtime.tree.{ParseTree, RuleNode, TerminalNode} | ||
|
|
||
| import org.apache.spark.SparkEnv | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.sql.AnalysisException | ||
| import org.apache.spark.sql.catalyst.{FunctionIdentifier, TableIdentifier} | ||
|
|
@@ -1229,25 +1228,56 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging | |
| CaseWhen(branches, Option(ctx.elseExpression).map(expression)) | ||
| } | ||
|
|
||
| def enableHiveSupportQuotedIdentifiers() : Boolean = { | ||
| SparkEnv.get != null && | ||
| SparkEnv.get.conf != null && | ||
| SparkEnv.get.conf.getBoolean("hive.support.quoted.identifiers", false) | ||
|
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. Spark SQL always supports quoted identifiers. However, the missing part is the
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. Added to SQLConf |
||
| } | ||
|
|
||
| /** | ||
| * Create a dereference expression. The return type depends on the type of the parent, this can | ||
| * either be a [[UnresolvedAttribute]] (if the parent is an [[UnresolvedAttribute]]), or an | ||
| * [[UnresolvedExtractValue]] if the parent is some expression. | ||
| * Create a dereference expression. The return type depends on the type of the parent. | ||
| * If the parent is an [[UnresolvedAttribute]], it can be a [[UnresolvedAttribute]] or | ||
| * a [[UnresolvedRegex]] for regex quoted in ``; if the parent is some other expression, | ||
| * it can be [[UnresolvedExtractValue]]. | ||
| */ | ||
| override def visitDereference(ctx: DereferenceContext): Expression = withOrigin(ctx) { | ||
| val attr = ctx.fieldName.getText | ||
| expression(ctx.base) match { | ||
| case UnresolvedAttribute(nameParts) => | ||
| case unresolved_attr @ UnresolvedAttribute(nameParts) => | ||
|
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. Please use a guard, e.g.:
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. It is concise to put the if inside the case unresolved_attr @ UnresolvedAttribute(nameParts). if we use guard, we still need to handle the case when the conf.supportQuotedIdentifiers is false.
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. how about
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. this wont work. In your first "case", ctx.fieldName.getStart.getText is
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. oh sorry I made a mistake,
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. yes, ctx.fieldName.getText will trim the backquote |
||
| if (enableHiveSupportQuotedIdentifiers) { | ||
| val escapedIdentifier = "`(.+)`".r | ||
|
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. We don't need to compile the same regex over and over. Can you move this to the ParserUtils... I am also wondering if we shouldn't do the match in the parser it self.
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. Add API in ParserUtils. I think in the parser, it can still get the backticks. that, the backticks are stripped off. |
||
| val ret = Option(ctx.fieldName.getStart).map(_.getText match { | ||
|
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. Using an option here does not add a thing.
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. removed the option |
||
| case r@escapedIdentifier(i) => | ||
| UnresolvedRegex(i, Some(unresolved_attr.name)) | ||
|
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. How about no change in the parser? Is that possible we can simply resolve it in BTW, we also need to handle the same issue in the Dataset APIs.
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. Before we go to ResolveReferences, we need to figure out that the regex is the special case, just like Star. ResolveReferences will do the resolve based on the expression type is UnresolvedRegx, Star etc. Database API goes the same path. I have added a unittest for DatasetSuite.scala for regex |
||
| case _ => | ||
| UnresolvedAttribute(nameParts :+ attr) | ||
| }) | ||
| return ret.get | ||
| } | ||
|
|
||
| UnresolvedAttribute(nameParts :+ attr) | ||
| case e => | ||
| UnresolvedExtractValue(e, Literal(attr)) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Create an [[UnresolvedAttribute]] expression. | ||
| * Create an [[UnresolvedAttribute]] expression or a [[UnresolvedRegex]] if it is a regex | ||
|
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. what if we always create
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. we should only create UnresolvedRegex when necessary.
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. there seems no problem if we always go with the
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 code complexity will be similar, because if the column is ``, we need to extract the pattern;
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'm not talking about algorithm complexity, but saying that we can simplify the logic by avoiding detecting the regex string.
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. @cloud-fan, the code path is shared by by both select a, select a.b and on cause etc. If it is select a.b, the table part also go here, but later there is no project expand. If it is on cause, the the string is already striped, not regex any more. Only with column names, we will have the project expanding (similar to start). So, we will need the regex pattern match to know that this is only for columns. Do you have any suggestion? Currently Hive only supports select column regex expansion. and this PR matches the hive behavior. |
||
| * quoted in `` | ||
| */ | ||
| override def visitColumnReference(ctx: ColumnReferenceContext): Expression = withOrigin(ctx) { | ||
| if (enableHiveSupportQuotedIdentifiers) { | ||
| val escapedIdentifier = "`(.+)`".r | ||
|
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. We don't need to compile the same regex over and over. Can you move this to the ParserUtils...
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. Add API in ParserUtils. |
||
| val ret = Option(ctx.getStart).map(_.getText match { | ||
|
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. Using an option here does not add a thing.
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. removed the option |
||
| case r @ escapedIdentifier(i) => | ||
| UnresolvedRegex(i, None) | ||
| case _ => | ||
| UnresolvedAttribute.quoted(ctx.getText) | ||
| }) | ||
|
|
||
| return ret.get | ||
| } | ||
|
|
||
| UnresolvedAttribute.quoted(ctx.getText) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2624,4 +2624,92 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext { | |
| val e = intercept[AnalysisException](sql("SELECT nvl(1, 2, 3)")) | ||
| assert(e.message.contains("Invalid number of arguments")) | ||
| } | ||
|
|
||
| test("SPARK-12139: REGEX Column Specification for Hive Queries") { | ||
|
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. Could you create a file in You can run
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. Yes let's use those rather than adding more files to SQLQuerySUite. I'd love to get rid of SQLQuerySuite ....
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. moved the test to sql/core/src/test/resources/sql-tests/inputs |
||
| // hive.support.quoted.identifiers is turned off by default | ||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT b | ||
| |FROM testData2 | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT t.b | ||
| |FROM testData2 t | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
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. The above two test queries are not needed in the new suite.
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. removed. I was trying to make sure that the existing behaviors are not broken. |
||
|
|
||
| intercept[AnalysisException] { | ||
| sql( | ||
| """ | ||
| |SELECT `(a)?+.+` | ||
| |FROM testData2 | ||
| |WHERE a = 1 | ||
| """.stripMargin) | ||
| } | ||
|
|
||
| intercept[AnalysisException] { | ||
| sql( | ||
| """ | ||
| |SELECT t.`(a)?+.+` | ||
| |FROM testData2 t | ||
| |WHERE a = 1 | ||
| """.stripMargin) | ||
| } | ||
|
|
||
| // now, turn on hive.support.quoted.identifiers | ||
| sparkContext.conf.set("hive.support.quoted.identifiers", "true") | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT b | ||
| |FROM testData2 | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT t.b | ||
| |FROM testData2 t | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT `(a)?+.+` | ||
| |FROM testData2 | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT t.`(a)?+.+` | ||
| |FROM testData2 t | ||
| |WHERE a = 1 | ||
| """.stripMargin), | ||
| Row(1) :: Row(2) :: Nil) | ||
|
|
||
| checkAnswer( | ||
| sql( | ||
| """ | ||
| |SELECT p.`(key)?+.+`, b, testdata2.`(b)?+.+` | ||
| |FROM testData p join testData2 | ||
| |ON p.key = testData2.a | ||
| |WHERE key < 3 | ||
| """.stripMargin), | ||
| Row("1", 1, 1) :: Row("1", 2, 1) :: Row("2", 1, 2) :: Row("2", 2, 2) ::Nil) | ||
| } | ||
| } | ||
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.
expris the pattern right? Maybe we should give it a better name.Uh oh!
There was an error while loading. Please reload this page.
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.
renamed to regexPattern