You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by skambha <gi...@git.apache.org> on 2017/03/07 04:47:30 UTC
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
GitHub user skambha opened a pull request:
https://github.com/apache/spark/pull/17185
[SPARK-19602][SQL] Support column resolution of fully qualified column name ( 3 part name)
## What changes were proposed in this pull request?
The design details is attached to the JIRA issue [here](https://issues.apache.org/jira/secure/attachment/12854681/Design_ColResolution_JIRA19602.pdf)
High level overview of the changes are:
- Enhance the qualifier to be more than one string
- Add support to store the qualifier. Enhance the lookupRelation to keep the qualifier appropriately.
- Enhance the table matching column resolution algorithm to account for qualifier being more than a string.
- Enhance the table matching algorithm in UnresolvedStar.expand
- Ensure that we continue to support select t1.i1 from db1.t1
## How was this patch tested?
- New tests are added.
- Several test scenarios were added in a separate [test pr 17067](https://github.com/apache/spark/pull/17067). The tests that were not supported earlier are marked with TODO markers and those are now supported with the code changes here.
- Existing unit tests ( hive, catalyst and sql) were run successfully.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/skambha/spark colResolution
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17185.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #17185
----
commit 9b27c0bb832444775f2e6f5488d66b8b4415350e
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2016-11-01T23:33:45Z
Changes to resolve fully qualified column attribute references from a logical plan node children
commit 66c004d46e6fa837b940c88384ff0bc11e34482c
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-03T23:07:16Z
Add AliasIdentifier, cleanups, merge and rebases
commit cd2e18bac541dc26b77af219e5120b29fbbcb87a
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-04T00:21:56Z
Tests now support qualified column name (3 part) and rebase changes
commit e23407fbb2e472d0dfc470afdb12c09cd4ca4d9f
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-05T01:14:10Z
merge changes
commit 0b3e6f8eb8f019404d292984a16a63a6c75ffe88
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-06T23:14:10Z
Cleanup
commit 8d6bd3f74a509614f1de81803ef207b1440eaa17
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-06T23:52:10Z
test cleanup
commit fb72118ce1b780deb0613578cf69320e16f12f5e
Author: Sunitha Kambhampati <sk...@us.ibm.com>
Date: 2017-03-07T00:12:35Z
formatting
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207759811
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
What I'm talking about is ambiguity. `col.innerField1.innerField2` can fail if the `innerField2` doesn't exist. My question is: shall we try all the possible resolution paths and pick the valid one, or define a rule that we can decide the resolution path ahead.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #93904 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93904/testReport)** for PR 17185 at commit [`05d6c0f`](https://github.com/apache/spark/commit/05d6c0fc6139f8672aea3173da3d98c5cc4e1a29).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207717536
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala ---
@@ -536,12 +536,13 @@ abstract class SessionCatalogSuite extends AnalysisTest {
assert(metadata.viewText.isDefined)
val view = View(desc = metadata, output = metadata.schema.toAttributes,
child = CatalystSqlParser.parsePlan(metadata.viewText.get))
+ val alias = AliasIdentifier("view1", Some("db3"))
--- End diff --
Sure, let me do that if this style is preferred. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208079529
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -201,7 +204,7 @@ case class Alias(child: Expression, name: String)(
}
override def sql: String = {
- val qualifierPrefix = qualifier.map(_ + ".").getOrElse("")
+ val qualifierPrefix = if (qualifier.nonEmpty) qualifier.mkString(".") + "." else ""
--- End diff --
ok, sounds good.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207726267
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
Actually it won't fail without this patch too, the col.innerField1.innerField2 will resolve correctly to the struct field if there is a table named col and column named innerField1 which has a innerField2 field.
Please see an example of such a scenario in this test output on master spark:
https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/results/columnresolution.sql.out#L360
------
Let me look into the sql standard and get back on that.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #93810 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93810/testReport)** for PR 17185 at commit [`8206dc3`](https://github.com/apache/spark/commit/8206dc3bc2595507ba71e7f50fddeed0c3b16479).
* This patch **fails Spark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r165230391
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -88,12 +88,12 @@ case class UnresolvedAttribute(nameParts: Seq[String]) extends Attribute with Un
override def exprId: ExprId = throw new UnresolvedException(this, "exprId")
override def dataType: DataType = throw new UnresolvedException(this, "dataType")
override def nullable: Boolean = throw new UnresolvedException(this, "nullable")
- override def qualifier: Option[String] = throw new UnresolvedException(this, "qualifier")
+ override def qualifier: Option[Seq[String]] = throw new UnresolvedException(this, "qualifier")
--- End diff --
I followed the style that exists today. If we change here, then there are other places where it needs to change as well. For e.g, in AttributeReference we initialize it with None today and now it will be a Seq.empty. I am not sure if we want to create these objects or just leave it using None by keeping the Option[]
If you have a strong preference, I can update it. I do have the changes locally but have not pushed that version. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208078928
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala ---
@@ -794,19 +795,37 @@ case class LocalLimit(limitExpr: Expression, child: LogicalPlan) extends OrderPr
/**
* Aliased subquery.
*
- * @param alias the alias name for this subquery.
+ * @param name the alias identifier for this subquery.
* @param child the logical plan of this subquery.
*/
case class SubqueryAlias(
- alias: String,
+ name: AliasIdentifier,
child: LogicalPlan)
extends OrderPreservingUnaryNode {
- override def doCanonicalize(): LogicalPlan = child.canonicalized
+ def alias: String = name.identifier
- override def output: Seq[Attribute] = child.output.map(_.withQualifier(Some(alias)))
+ override def output: Seq[Attribute] = {
+ val qualifierList = name.database.map(Seq(_, alias)).getOrElse(Seq(alias))
+ child.output.map(_.withQualifier(qualifierList))
+ }
+ override def doCanonicalize(): LogicalPlan = child.canonicalized
}
+object SubqueryAlias {
+ def apply(
+ identifier: String,
+ child: LogicalPlan): SubqueryAlias = {
+ SubqueryAlias(AliasIdentifier(identifier), child)
+ }
+
+ def apply(
+ identifier: String,
+ database: Option[String],
--- End diff --
good point! I'll take care of this in the next push.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208078754
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -201,7 +204,7 @@ case class Alias(child: Expression, name: String)(
}
override def sql: String = {
- val qualifierPrefix = qualifier.map(_ + ".").getOrElse("")
+ val qualifierPrefix = if (qualifier.nonEmpty) qualifier.mkString(".") + "." else ""
--- End diff --
This won't work for the case when we have Seq.empty. The suffix "." gets returned even for a empty sequence.
For a non empty Seq, the above call will be fine.
Shall we leave the 'if' as is or is there an equivalent preferred style that would work?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17185
overall LGTM, my major concern is how to do O(1) lookup for the 3 part name
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208059884
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
@cloud-fan , Thank you for your suggestion and question.
Existing spark behavior follows precedence rules in the column resolution logic and in this patch we are following the same pattern/rule.
I am looking into the SQL standard to see if there are any column resolution rules but I have not found any yet. However when I researched existing databases, I observed different behaviors among them and it is listed in Section 2/Table A in the design doc [here](https://drive.google.com/file/d/1zKm3aNZ3DpsqIuoMvRsf0kkDkXsAasxH/view).
I agree, we can improve upon the checks in existing precedence to go all the way to ensure there is a nested field. Although, the user can always qualify the field to resolve the ambiguity. Shall we open another issue to discuss and improve upon the existing resolution logic.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94187 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94187/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94162 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94162/testReport)** for PR 17185 at commit [`8931354`](https://github.com/apache/spark/commit/8931354b01297833bda1dbcff21d42f928448b3a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206988504
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -120,22 +120,54 @@ abstract class LogicalPlan
/**
* Resolve the given `name` string against the given attribute, returning either 0 or 1 match.
--- End diff --
At a high level the changes include:
* The column resolution needs to account for the qualifier that is now a sequence of string.
* Enhance the lookupRelation to add the qualifier appropriately. Right now, the qualifier used by the LogicalPlan nodes representing the relation leafnodes use only the table name as the qualifier when no alias is provided. This needs to be updated to add the database name and the table name for permanent views and tables and global temporary views.
Full details of the design and the resolution logic is in the [doc here](https://drive.google.com/file/d/1zKm3aNZ3DpsqIuoMvRsf0kkDkXsAasxH/view) in Section 3.3 and 3.4.
So, Section 2 in the design doc has a lot of scenarios that were tested against different db’s and Spark and the expected behavior with the design here.
If there is a specific scenario you have in mind that would be valid but get flagged as ambiguous, can you please let me know. I’d be glad to look into it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r163322810
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -88,12 +88,12 @@ case class UnresolvedAttribute(nameParts: Seq[String]) extends Attribute with Un
override def exprId: ExprId = throw new UnresolvedException(this, "exprId")
override def dataType: DataType = throw new UnresolvedException(this, "dataType")
override def nullable: Boolean = throw new UnresolvedException(this, "nullable")
- override def qualifier: Option[String] = throw new UnresolvedException(this, "qualifier")
+ override def qualifier: Option[Seq[String]] = throw new UnresolvedException(this, "qualifier")
--- End diff --
I think we can just use `Seq[String]`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207717569
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -316,8 +345,8 @@ case class UnresolvedRegex(regexPattern: String, table: Option[String], caseSens
// If there is no table specified, use all input attributes that match expr
case None => input.output.filter(_.name.matches(pattern))
// If there is a table, pick out attributes that are part of this table that match expr
- case Some(t) => input.output.filter(_.qualifier.exists(resolver(_, t)))
- .filter(_.name.matches(pattern))
+ case Some(t) => input.output.filter(a => resolver(a.qualifier.last, t)).
--- End diff --
Sure. Will add a defensive check.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94234/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
I rebased and found out that the resolution code in Logical plan has changed and it uses map lookup to do the matching. I have some ideas on how to incorporate the 3 part name with the map lookup logic.
For now, I have synced up and bypassed the new map logic and pushed the code so it is up to the latest so I can get the test cycle from GitHub and added todo’s for myself to incorporate the optimized lookup logic.
@cloud-fan, In the meantime, if you have any other comments please let me know. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r104817091
--- Diff: sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql ---
@@ -13,10 +13,8 @@ DROP VIEW view1;
-- Test scenario with Global Temp view
CREATE OR REPLACE GLOBAL TEMPORARY VIEW view1 as SELECT 1 as i1;
SELECT * FROM global_temp.view1;
--- TODO: Support this scenario
SELECT global_temp.view1.* FROM global_temp.view1;
--- End diff --
I'm not sure if this is a valid case. We should treat `SELECT ... FROM ...` as a 2-levels look up: Level 1, in `FROM` statement, we specify the table name with an optional qualifier(database name). Level 2, in `SELECT` statement, we specify the column name with an optional qualifier(table name).
I don't think it makes sense to specify the database name for column name, the table lookup should be done in the `FROM` statement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94234 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94234/testReport)** for PR 17185 at commit [`065687f`](https://github.com/apache/spark/commit/065687f3b987e254f412796666d45f0cced6e42e).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
I rebased and ran the catalyst unit test suite only as a sanity test. fwiw, I had run the sql and hive and catalyst suites earlier prior to this last rebase.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94162 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94162/testReport)** for PR 17185 at commit [`8931354`](https://github.com/apache/spark/commit/8931354b01297833bda1dbcff21d42f928448b3a).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17185
LGTM except some code style comment, thanks for working on it!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #93904 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93904/testReport)** for PR 17185 at commit [`05d6c0f`](https://github.com/apache/spark/commit/05d6c0fc6139f8672aea3173da3d98c5cc4e1a29).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208074858
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala ---
@@ -794,19 +795,37 @@ case class LocalLimit(limitExpr: Expression, child: LogicalPlan) extends OrderPr
/**
* Aliased subquery.
*
- * @param alias the alias name for this subquery.
+ * @param name the alias identifier for this subquery.
* @param child the logical plan of this subquery.
*/
case class SubqueryAlias(
- alias: String,
+ name: AliasIdentifier,
child: LogicalPlan)
extends OrderPreservingUnaryNode {
- override def doCanonicalize(): LogicalPlan = child.canonicalized
+ def alias: String = name.identifier
- override def output: Seq[Attribute] = child.output.map(_.withQualifier(Some(alias)))
+ override def output: Seq[Attribute] = {
+ val qualifierList = name.database.map(Seq(_, alias)).getOrElse(Seq(alias))
+ child.output.map(_.withQualifier(qualifierList))
+ }
+ override def doCanonicalize(): LogicalPlan = child.canonicalized
}
+object SubqueryAlias {
+ def apply(
+ identifier: String,
+ child: LogicalPlan): SubqueryAlias = {
+ SubqueryAlias(AliasIdentifier(identifier), child)
+ }
+
+ def apply(
+ identifier: String,
+ database: Option[String],
--- End diff --
why it's `Option`? if no database name, we should call the `apply` method above
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
I have rebased and pushed the changes. I ran the unit tests ( sql, catalyst and hive).
Earlier, I was having issues running the hive test suite locally but that is resolved with the fix from SPARK-23275.
Please take a look. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/17185
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/17185
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94172 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94172/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
The last push has changes that adds the map lookup for the 3 part name. It implements a solution to address the lookup.. although I think there are more ways we can go about it and could possibly improve on it in the future.
@cloud-fan, please take a look. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207105608
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -71,19 +71,27 @@ trait NamedExpression extends Expression {
* multiple qualifiers, it is possible that there are other possible way to refer to this
* attribute.
*/
- def qualifiedName: String = (qualifier.toSeq :+ name).mkString(".")
+ def qualifiedName: String = {
+ if (qualifier.isDefined) {
+ (qualifier.get :+ name).mkString(".")
+ } else {
+ name
+ }
+ }
/**
* Optional qualifier for the expression.
+ * Qualifier can also contain the fully qualified information, for e.g, Sequence of string
+ * containing the database and the table name
*
* For now, since we do not allow using original table name to qualify a column name once the
* table is aliased, this can only be:
*
* 1. Empty Seq: when an attribute doesn't have a qualifier,
* e.g. top level attributes aliased in the SELECT clause, or column from a LocalRelation.
- * 2. Single element: either the table name or the alias name of the table.
+ * 2. Seq with a Single element: either the table name or the alias name of the table.
--- End diff --
3. a seq of 2 elements: database name and table name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94160 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94160/testReport)** for PR 17185 at commit [`4e9c1bf`](https://github.com/apache/spark/commit/4e9c1bfede87060cd89f227e1891d9a0046c8df9).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94353 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94353/testReport)** for PR 17185 at commit [`5f7e5d7`](https://github.com/apache/spark/commit/5f7e5d7bddca593d72818b07d71f678bd0a1982d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #93810 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/93810/testReport)** for PR 17185 at commit [`8206dc3`](https://github.com/apache/spark/commit/8206dc3bc2595507ba71e7f50fddeed0c3b16479).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207702706
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala ---
@@ -536,12 +536,13 @@ abstract class SessionCatalogSuite extends AnalysisTest {
assert(metadata.viewText.isDefined)
val view = View(desc = metadata, output = metadata.schema.toAttributes,
child = CatalystSqlParser.parsePlan(metadata.viewText.get))
+ val alias = AliasIdentifier("view1", Some("db3"))
--- End diff --
instead of creating `AliasIdentifier` manually, we can also define a `SubqueryAlias.apply` which takes db name and table name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206975896
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
--- End diff --
This (SELECT db1.t1.* FROM t1) would resolve correctly.
Also a similar scenario (details in Section 2 in Table A # 5 in the design doc)
select db1.t1.i1 from t1 will resolve correctly when the current database is db1.
I will explicitly add a test case for the db1.t1.* from t1 as well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
@skambha @dilipbiswal How about the hint resolution after supporting multi part names?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94234 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94234/testReport)** for PR 17185 at commit [`065687f`](https://github.com/apache/spark/commit/065687f3b987e254f412796666d45f0cced6e42e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207104014
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
+ * qualifier of the attribute is Seq("db1","t1")
+ * ii) If (i) is not true, then check if nameParts is only a single element and it
+ * matches the table portion of the qualifier
+ *
+ * E.g. SELECT t1.* FROM db1.t1 In this case nameParts is Seq("t1") and
+ * qualifier is Seq("db1","t1")
+ * SELECT a.* FROM db1.t1 AS a
+ * In this case nameParts is Seq("a") and qualifier for
+ * attribute is Seq("a")
+ */
+ private def matchedQualifier(
+ attribute: Attribute,
--- End diff --
do we assume the attribute always carry the full qualifier? and i.e. the analyzer will always include the current database in the qualifier.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207725519
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
Is this behavior defined by SQL standard? IIRC the current behavior without your patch is
`col.innerField1.innerField2` can fail even we have a table named `col` and a column named `innerField1` which has a `innerField2` field.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208074745
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
+ // Attribute(b, qualifier("db1,"a")) and List("c") will be the second element
+ var matches: (Seq[Attribute], Seq[String]) = nameParts match {
+ case dbPart +: tblPart +: name +: nestedFields =>
+ val key = (dbPart.toLowerCase(Locale.ROOT),
+ tblPart.toLowerCase(Locale.ROOT), name.toLowerCase(Locale.ROOT))
+ val attributes = collectMatches(name, qualified3Part.get(key)).filter {
+ a => (resolver(dbPart, a.qualifier.head) && resolver(tblPart, a.qualifier.last))
+ }
+ (attributes, nestedFields)
+ case all =>
+ (Seq.empty, Seq.empty)
+ }
+
+ // If there are no matches, then find matches for the given name assuming that
+ // the 1st part is a qualifier (i.e. table name, alias, or subquery alias) and the
+ // 2nd part is the actual name. This returns a tuple of
// matched attributes and a list of parts that are to be resolved.
//
// For example, consider an example where "a" is the table name, "b" is the column name,
// and "c" is the struct field name, i.e. "a.b.c". In this case, Attribute will be "a.b",
// and the second element will be List("c").
- val matches = nameParts match {
- case qualifier +: name +: nestedFields =>
- val key = (qualifier.toLowerCase(Locale.ROOT), name.toLowerCase(Locale.ROOT))
- val attributes = collectMatches(name, qualified.get(key)).filter { a =>
- resolver(qualifier, a.qualifier.get)
+ matches = matches match {
--- End diff --
nit:
```
if (matches._1.isEmpty) {
matches = ...
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206382570
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -120,22 +120,54 @@ abstract class LogicalPlan
/**
* Resolve the given `name` string against the given attribute, returning either 0 or 1 match.
--- End diff --
can you briefly explain the new resolution logic? I feel it's a little convoluted now as we are more likely to be ambiguous.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
@gatorsmile , @cloud-fan, just a quick comment, I have been working on this and will respond soon.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206381686
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -121,14 +129,14 @@ abstract class Attribute extends LeafExpression with NamedExpression with NullIn
* @param name The name to be associated with the result of computing [[child]].
* @param exprId A globally unique id used to check if an [[AttributeReference]] refers to this
* alias. Auto-assigned if left blank.
- * @param qualifier An optional string that can be used to referred to this attribute in a fully
- * qualified way. Consider the examples tableName.name, subQueryAlias.name.
- * tableName and subQueryAlias are possible qualifiers.
+ * @param qualifier An optional Seq of string that can be used to refer to this attribute in a
+ * fully qualified way. Consider the examples tableName.name, subQueryAlias.name.
+ * tableName and subQueryAlias are possible qualifiers.
* @param explicitMetadata Explicit metadata associated with this alias that overwrites child's.
*/
case class Alias(child: Expression, name: String)(
val exprId: ExprId = NamedExpression.newExprId,
- val qualifier: Option[String] = None,
+ val qualifier: Option[Seq[String]] = None,
--- End diff --
Again, I feel using `Seq[String]` directly can simplify a lot of code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206381459
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala ---
@@ -654,16 +654,19 @@ class SessionCatalog(
*
* If the relation is a view, we generate a [[View]] operator from the view description, and
* wrap the logical plan in a [[SubqueryAlias]] which will track the name of the view.
+ * [[SubqueryAlias]] will also keep track of the name and database(optional) of the table/view
*
* @param name The name of the table/view that we look up.
*/
def lookupRelation(name: TableIdentifier): LogicalPlan = {
synchronized {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
+ // To keep track of the name and database of the table/view
+ val alias = AliasIdentifier(table, Some(db))
if (db == globalTempViewManager.database) {
globalTempViewManager.get(table).map { viewDef =>
- SubqueryAlias(table, viewDef)
+ SubqueryAlias(alias, viewDef)
--- End diff --
I think we can make `SubqueryAlias` take a `qualifier: Seq[String]` instead of `alias: String`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207640501
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
+ * qualifier of the attribute is Seq("db1","t1")
+ * ii) If (i) is not true, then check if nameParts is only a single element and it
+ * matches the table portion of the qualifier
+ *
+ * E.g. SELECT t1.* FROM db1.t1 In this case nameParts is Seq("t1") and
+ * qualifier is Seq("db1","t1")
+ * SELECT a.* FROM db1.t1 AS a
+ * In this case nameParts is Seq("a") and qualifier for
+ * attribute is Seq("a")
+ */
+ private def matchedQualifier(
+ attribute: Attribute,
+ nameParts: Seq[String],
+ resolver: Resolver): Boolean = {
+ val qualifierList = attribute.qualifier.getOrElse(Seq.empty)
+
+ // match the qualifiers and nameParts
+ val matched = nameParts.corresponds(qualifierList)(resolver) match {
--- End diff --
done. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #86899 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86899/testReport)** for PR 17185 at commit [`8206dc3`](https://github.com/apache/spark/commit/8206dc3bc2595507ba71e7f50fddeed0c3b16479).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r104817117
--- Diff: sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql ---
@@ -13,10 +13,8 @@ DROP VIEW view1;
-- Test scenario with Global Temp view
CREATE OR REPLACE GLOBAL TEMPORARY VIEW view1 as SELECT 1 as i1;
SELECT * FROM global_temp.view1;
--- TODO: Support this scenario
SELECT global_temp.view1.* FROM global_temp.view1;
--- End diff --
cc @gatorsmile
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by mafernandez-stratio <gi...@git.apache.org>.
Github user mafernandez-stratio commented on the issue:
https://github.com/apache/spark/pull/17185
@gatorsmile Great, thanks for the feedback!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93810/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17185
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
retest this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207104518
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
+ * qualifier of the attribute is Seq("db1","t1")
+ * ii) If (i) is not true, then check if nameParts is only a single element and it
+ * matches the table portion of the qualifier
+ *
+ * E.g. SELECT t1.* FROM db1.t1 In this case nameParts is Seq("t1") and
+ * qualifier is Seq("db1","t1")
+ * SELECT a.* FROM db1.t1 AS a
+ * In this case nameParts is Seq("a") and qualifier for
+ * attribute is Seq("a")
+ */
+ private def matchedQualifier(
+ attribute: Attribute,
+ nameParts: Seq[String],
+ resolver: Resolver): Boolean = {
+ val qualifierList = attribute.qualifier.getOrElse(Seq.empty)
+
+ // match the qualifiers and nameParts
+ val matched = nameParts.corresponds(qualifierList)(resolver) match {
+ case true => true
+ case false if (nameParts.length == 1 && qualifierList.nonEmpty) =>
+ // check if it matches the table portion of the qualifier
--- End diff --
IIRC users can do `select structCol.* from tbl`, does it mean the `nameParts.head` may not be table name?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94244 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94244/testReport)** for PR 17185 at commit [`065687f`](https://github.com/apache/spark/commit/065687f3b987e254f412796666d45f0cced6e42e).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
Thanks @cloud-fan for the review.
I am working on implementing an idea to get optimized lookup with 3part name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94244 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94244/testReport)** for PR 17185 at commit [`065687f`](https://github.com/apache/spark/commit/065687f3b987e254f412796666d45f0cced6e42e).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207635526
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
+ * qualifier of the attribute is Seq("db1","t1")
+ * ii) If (i) is not true, then check if nameParts is only a single element and it
+ * matches the table portion of the qualifier
+ *
+ * E.g. SELECT t1.* FROM db1.t1 In this case nameParts is Seq("t1") and
+ * qualifier is Seq("db1","t1")
+ * SELECT a.* FROM db1.t1 AS a
+ * In this case nameParts is Seq("a") and qualifier for
+ * attribute is Seq("a")
+ */
+ private def matchedQualifier(
+ attribute: Attribute,
--- End diff --
The database information gets carried over based on the lookupRelation logic. Details are in Section 3.3.3 in the design doc [here](https://drive.google.com/file/d/1zKm3aNZ3DpsqIuoMvRsf0kkDkXsAasxH/view)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94353 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94353/testReport)** for PR 17185 at commit [`5f7e5d7`](https://github.com/apache/spark/commit/5f7e5d7bddca593d72818b07d71f678bd0a1982d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94162/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94335 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94335/testReport)** for PR 17185 at commit [`5f7e5d7`](https://github.com/apache/spark/commit/5f7e5d7bddca593d72818b07d71f678bd0a1982d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94335 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94335/testReport)** for PR 17185 at commit [`5f7e5d7`](https://github.com/apache/spark/commit/5f7e5d7bddca593d72818b07d71f678bd0a1982d).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
Let us discuss it in the JIRA https://issues.apache.org/jira/browse/SPARK-25121
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/93904/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94206/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r106253035
--- Diff: sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql ---
@@ -13,10 +13,8 @@ DROP VIEW view1;
-- Test scenario with Global Temp view
CREATE OR REPLACE GLOBAL TEMPORARY VIEW view1 as SELECT 1 as i1;
SELECT * FROM global_temp.view1;
--- TODO: Support this scenario
SELECT global_temp.view1.* FROM global_temp.view1;
--- End diff --
Thanks @gatorsmile and @cloud-fan.
Hi @cloud-fan,
The use-case is to support fully qualified column name in queries to remove ambiguity in the naming.
For e.g. `select db2.t1.i1 from db1.t1, db2.t1 where db1.t1.i2 = db2.t1.i3`
Today Spark does not support this. The workaround is to create alias for the table.
Yes, the lookup for the table is coming from the \u2018from\u2019 clause. At a high level, changes include keeping track of the qualifier(including the database name) when the lookup happens and enhancing the column resolution logic in the analyzer to resolve fully qualified column name from a logical plan node\u2019s children.
There are more details in the design doc attached to the[ SPARK-19602](https://issues.apache.org/jira/browse/SPARK-19602)
Thanks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208073035
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -201,7 +204,7 @@ case class Alias(child: Expression, name: String)(
}
override def sql: String = {
- val qualifierPrefix = qualifier.map(_ + ".").getOrElse("")
+ val qualifierPrefix = if (qualifier.nonEmpty) qualifier.mkString(".") + "." else ""
--- End diff --
nit: `val qualifierPrefix = qualifier.mkString("", ".", ".")`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207624865
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -121,14 +129,14 @@ abstract class Attribute extends LeafExpression with NamedExpression with NullIn
* @param name The name to be associated with the result of computing [[child]].
* @param exprId A globally unique id used to check if an [[AttributeReference]] refers to this
* alias. Auto-assigned if left blank.
- * @param qualifier An optional string that can be used to referred to this attribute in a fully
- * qualified way. Consider the examples tableName.name, subQueryAlias.name.
- * tableName and subQueryAlias are possible qualifiers.
+ * @param qualifier An optional Seq of string that can be used to refer to this attribute in a
+ * fully qualified way. Consider the examples tableName.name, subQueryAlias.name.
+ * tableName and subQueryAlias are possible qualifiers.
* @param explicitMetadata Explicit metadata associated with this alias that overwrites child's.
*/
case class Alias(child: Expression, name: String)(
val exprId: ExprId = NamedExpression.newExprId,
- val qualifier: Option[String] = None,
+ val qualifier: Option[Seq[String]] = None,
--- End diff --
I took care of this change in the recent push. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94244/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207718090
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
In this case, if a.b.c fails to resolve as db.table.column, then we check if there is a table and column that matches a.b and then see if c is a nested field name and if it exists, it will resolve to the nested field.
Tests with struct nested fields are [here](https://github.com/skambha/spark/blob/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8/sql/core/src/test/resources/sql-tests/inputs/columnresolution.sql#L65-L73)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17185
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
I have addressed the review comments in this commit [here](https://github.com/apache/spark/pull/17185/commits/065687f3b987e254f412796666d45f0cced6e42e)
@cloud-fan, please take a look. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17185
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94160 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94160/testReport)** for PR 17185 at commit [`4e9c1bf`](https://github.com/apache/spark/commit/4e9c1bfede87060cd89f227e1891d9a0046c8df9).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94206 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94206/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94206 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94206/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17185
I agree it's a valid use case, do you wanna bring it up to date? sorry for the delay!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
@mafernandez-stratio We plan to review and merge after 2.3 release. The target release will be 2.4
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207701939
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -316,8 +345,8 @@ case class UnresolvedRegex(regexPattern: String, table: Option[String], caseSens
// If there is no table specified, use all input attributes that match expr
case None => input.output.filter(_.name.matches(pattern))
// If there is a table, pick out attributes that are part of this table that match expr
- case Some(t) => input.output.filter(_.qualifier.exists(resolver(_, t)))
- .filter(_.name.matches(pattern))
+ case Some(t) => input.output.filter(a => resolver(a.qualifier.last, t)).
--- End diff --
do we need to consider if `qualifier` is Nil?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/17185
Good idea @gatorsmile . Thanks !!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #86899 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/86899/testReport)** for PR 17185 at commit [`8206dc3`](https://github.com/apache/spark/commit/8206dc3bc2595507ba71e7f50fddeed0c3b16479).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206978220
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala ---
@@ -654,16 +654,19 @@ class SessionCatalog(
*
* If the relation is a view, we generate a [[View]] operator from the view description, and
* wrap the logical plan in a [[SubqueryAlias]] which will track the name of the view.
+ * [[SubqueryAlias]] will also keep track of the name and database(optional) of the table/view
*
* @param name The name of the table/view that we look up.
*/
def lookupRelation(name: TableIdentifier): LogicalPlan = {
synchronized {
val db = formatDatabaseName(name.database.getOrElse(currentDb))
val table = formatTableName(name.table)
+ // To keep track of the name and database of the table/view
+ val alias = AliasIdentifier(table, Some(db))
if (db == globalTempViewManager.database) {
globalTempViewManager.get(table).map { viewDef =>
- SubqueryAlias(table, viewDef)
+ SubqueryAlias(alias, viewDef)
--- End diff --
Here, SubqueryAlias takes in a AliasIdentifier and not a string.
Also want to mention that: we considered somewhat similar to your suggestion - to simply add a qualifier as a Seq[String] as a member of SubqueryAlias but that change would require touching more files.
I had implemented that first (long time back now) and then after some offline discussions we favored this as this change is more localized and encapsulated. @gatorsmile @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207624726
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -71,19 +71,27 @@ trait NamedExpression extends Expression {
* multiple qualifiers, it is possible that there are other possible way to refer to this
* attribute.
*/
- def qualifiedName: String = (qualifier.toSeq :+ name).mkString(".")
+ def qualifiedName: String = {
+ if (qualifier.isDefined) {
+ (qualifier.get :+ name).mkString(".")
+ } else {
+ name
+ }
+ }
/**
* Optional qualifier for the expression.
+ * Qualifier can also contain the fully qualified information, for e.g, Sequence of string
+ * containing the database and the table name
*
* For now, since we do not allow using original table name to qualify a column name once the
* table is aliased, this can only be:
*
* 1. Empty Seq: when an attribute doesn't have a qualifier,
* e.g. top level attributes aliased in the SELECT clause, or column from a LocalRelation.
- * 2. Single element: either the table name or the alias name of the table.
+ * 2. Seq with a Single element: either the table name or the alias name of the table.
--- End diff --
done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r104822673
--- Diff: sql/core/src/test/resources/sql-tests/inputs/columnresolution-views.sql ---
@@ -13,10 +13,8 @@ DROP VIEW view1;
-- Test scenario with Global Temp view
CREATE OR REPLACE GLOBAL TEMPORARY VIEW view1 as SELECT 1 as i1;
SELECT * FROM global_temp.view1;
--- TODO: Support this scenario
SELECT global_temp.view1.* FROM global_temp.view1;
--- End diff --
Yes, but @skambha did a comparison between different DB. Basically, what she tried to do is to make Spark SQL consistent with the others. See the design doc:
https://issues.apache.org/jira/secure/attachment/12854681/Design_ColResolution_JIRA19602.pdf
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94335/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/17185
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
Thanks for the review. I have addressed your comments and pushed the changes.
@cloud-fan, Please take a look.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94187 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94187/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206380665
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
--- End diff --
what about `SELECT db1.t1.* FROM t1` while the current database is `db1`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207702684
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
--- End diff --
we may need to check the SQL standard about how to resolve the ambiguity. For `a.b.c`, it's possible that `a` is db name, `b` is table name, `c` is column name. If there is no column named `c`, we should fail. It's also possible that `a` is table name, `b` is column name, `c` is nested field name, and it exists. What's the expected behavior here?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
cc @gatorsmile, @cloud-fan I'd really appreciate your review and comments. Thanks much.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208079335
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -201,7 +204,7 @@ case class Alias(child: Expression, name: String)(
}
override def sql: String = {
- val qualifierPrefix = qualifier.map(_ + ".").getOrElse("")
+ val qualifierPrefix = if (qualifier.nonEmpty) qualifier.mkString(".") + "." else ""
--- End diff --
ah my bad, I thought it would return empty string for empty seq. Let's leave it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94172/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94160/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by mafernandez-stratio <gi...@git.apache.org>.
Github user mafernandez-stratio commented on the issue:
https://github.com/apache/spark/pull/17185
Any plans of merging this PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207027392
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/namedExpressions.scala ---
@@ -121,14 +129,14 @@ abstract class Attribute extends LeafExpression with NamedExpression with NullIn
* @param name The name to be associated with the result of computing [[child]].
* @param exprId A globally unique id used to check if an [[AttributeReference]] refers to this
* alias. Auto-assigned if left blank.
- * @param qualifier An optional string that can be used to referred to this attribute in a fully
- * qualified way. Consider the examples tableName.name, subQueryAlias.name.
- * tableName and subQueryAlias are possible qualifiers.
+ * @param qualifier An optional Seq of string that can be used to refer to this attribute in a
+ * fully qualified way. Consider the examples tableName.name, subQueryAlias.name.
+ * tableName and subQueryAlias are possible qualifiers.
* @param explicitMetadata Explicit metadata associated with this alias that overwrites child's.
*/
case class Alias(child: Expression, name: String)(
val exprId: ExprId = NamedExpression.newExprId,
- val qualifier: Option[String] = None,
+ val qualifier: Option[Seq[String]] = None,
--- End diff --
I'll look into this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r206985225
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LogicalPlan.scala ---
@@ -120,22 +120,54 @@ abstract class LogicalPlan
/**
* Resolve the given `name` string against the given attribute, returning either 0 or 1 match.
--- End diff --
The resolution logic needs to account to compare the full qualifier. If you are trying to resolve db1.t1.i1, the match needs to happen against the entire qualifier of the attribute.
For e.g:
```SQL
select db1.t1.i1 from t1 // current database is db1 and lets say t1 has columns i1, i2.
```
When resolving db1.t1.i1, against the following set of Input attributes.
we will have the following attribute references:
- AttributeReference ( qualifier Seq(db1, t1) and name is i1)
- AttributeReference ( qualifier Seq(db1, t1) and name is i2)
So the new resolution logic will match the entire qualifier sequence and then match the column ( ie the attribute name)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
sure. Let me look into it. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17185
**[Test build #94172 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94172/testReport)** for PR 17185 at commit [`90cd6d3`](https://github.com/apache/spark/commit/90cd6d33f59fdae16a3a386ed14cefb3f28d35a8).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/86899/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on the issue:
https://github.com/apache/spark/pull/17185
Looks like there is another change that has gone in that this PR conflicts with so the build fails. :(
I will rebase again and push.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by skambha <gi...@git.apache.org>.
Github user skambha commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r208089990
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/package.scala ---
@@ -169,25 +181,50 @@ package object expressions {
})
}
- // Find matches for the given name assuming that the 1st part is a qualifier (i.e. table name,
- // alias, or subquery alias) and the 2nd part is the actual name. This returns a tuple of
+ // Find matches for the given name assuming that the 1st two parts are qualifier
+ // (i.e. database name and table name) and the 3rd part is the actual column name.
+ //
+ // For example, consider an example where "db1" is the database name, "a" is the table name
+ // and "b" is the column name and "c" is the struct field name.
+ // If the name parts is db1.a.b.c, then Attribute will match
+ // Attribute(b, qualifier("db1,"a")) and List("c") will be the second element
+ var matches: (Seq[Attribute], Seq[String]) = nameParts match {
+ case dbPart +: tblPart +: name +: nestedFields =>
+ val key = (dbPart.toLowerCase(Locale.ROOT),
+ tblPart.toLowerCase(Locale.ROOT), name.toLowerCase(Locale.ROOT))
+ val attributes = collectMatches(name, qualified3Part.get(key)).filter {
+ a => (resolver(dbPart, a.qualifier.head) && resolver(tblPart, a.qualifier.last))
+ }
+ (attributes, nestedFields)
+ case all =>
+ (Seq.empty, Seq.empty)
+ }
+
+ // If there are no matches, then find matches for the given name assuming that
+ // the 1st part is a qualifier (i.e. table name, alias, or subquery alias) and the
+ // 2nd part is the actual name. This returns a tuple of
// matched attributes and a list of parts that are to be resolved.
//
// For example, consider an example where "a" is the table name, "b" is the column name,
// and "c" is the struct field name, i.e. "a.b.c". In this case, Attribute will be "a.b",
// and the second element will be List("c").
- val matches = nameParts match {
- case qualifier +: name +: nestedFields =>
- val key = (qualifier.toLowerCase(Locale.ROOT), name.toLowerCase(Locale.ROOT))
- val attributes = collectMatches(name, qualified.get(key)).filter { a =>
- resolver(qualifier, a.qualifier.get)
+ matches = matches match {
--- End diff --
done.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17185: [SPARK-19602][SQL] Support column resolution of f...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/17185#discussion_r207104432
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala ---
@@ -262,17 +262,47 @@ abstract class Star extends LeafExpression with NamedExpression {
*/
case class UnresolvedStar(target: Option[Seq[String]]) extends Star with Unevaluable {
- override def expand(input: LogicalPlan, resolver: Resolver): Seq[NamedExpression] = {
+ /**
+ * Returns true if the nameParts match the qualifier of the attribute
+ *
+ * There are two checks: i) Check if the nameParts match the qualifier fully.
+ * E.g. SELECT db.t1.* FROM db1.t1 In this case, the nameParts is Seq("db1", "t1") and
+ * qualifier of the attribute is Seq("db1","t1")
+ * ii) If (i) is not true, then check if nameParts is only a single element and it
+ * matches the table portion of the qualifier
+ *
+ * E.g. SELECT t1.* FROM db1.t1 In this case nameParts is Seq("t1") and
+ * qualifier is Seq("db1","t1")
+ * SELECT a.* FROM db1.t1 AS a
+ * In this case nameParts is Seq("a") and qualifier for
+ * attribute is Seq("a")
+ */
+ private def matchedQualifier(
+ attribute: Attribute,
+ nameParts: Seq[String],
+ resolver: Resolver): Boolean = {
+ val qualifierList = attribute.qualifier.getOrElse(Seq.empty)
+
+ // match the qualifiers and nameParts
+ val matched = nameParts.corresponds(qualifierList)(resolver) match {
--- End diff --
it's weird to use pattern match like this, maybe better to write
```
nameParts.corresponds(qualifierList)(resolver) || {
if (nameParts.length == 1 && qualifierList.nonEmpty) {
resolver(nameParts.head, qualifierList.last)
} else {
false
}
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94187/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17185: [SPARK-19602][SQL] Support column resolution of fully qu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17185
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94353/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org