-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-36895][SQL][FOLLOWUP] Use property to specify index type #34486
Conversation
Test build #144909 has finished for PR 34486 at commit
|
Kubernetes integration test starting |
Kubernetes integration test status failure |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #144910 has finished for PR 34486 at commit
|
Test build #144911 has finished for PR 34486 at commit
|
Test build #144912 has finished for PR 34486 at commit
|
|
||
CreateIndex( | ||
createUnresolvedTable(ctx.multipartIdentifier(), "CREATE INDEX"), | ||
indexName, | ||
indexType, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the plan unchanged. We should put indexType
in the properties in CreateIndexExec
.
Also let's not hardcode the property name. We can add a static constant in SupportsIndex
, similar to TableCatalog.PROP_PROVIDER
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #144938 has finished for PR 34486 at commit
|
// MySQL doesn't allow property set on individual column, so use empty Array for | ||
// column properties | ||
jdbcTable.createIndex("i1", "BTREE", Array(FieldReference("col1")), | ||
jdbcTable.createIndex("i1", Array(FieldReference("col1")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH it's a bit weird to write UT in this suite. Can we turn it into end-to-end test now as we have the SQL syntax ready?
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/index/SupportsIndex.java
Outdated
Show resolved
Hide resolved
table match { | ||
case s: SupportsIndex => | ||
val propertiesWithIndexType: Map[String, String] = if (indexType.nonEmpty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we put it inside CreateIndexExec
?
table match { | ||
case s: SupportsIndex => | ||
val propertiesWithIndexType: Map[String, String] = if (indexType.nonEmpty) { | ||
properties + ("indexType" -> indexType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not hardcode the property name
…log/index/SupportsIndex.java Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
Test build #144984 has finished for PR 34486 at commit
|
Kubernetes integration test unable to build dist. exiting with code: 1 |
@@ -432,7 +432,7 @@ class DataSourceV2Strategy(session: SparkSession) extends Strategy with Predicat | |||
AlterTableExec(table.catalog, table.identifier, a.changes) :: Nil | |||
|
|||
case CreateIndex(ResolvedTable(_, _, table, _), | |||
indexName, indexType, ifNotExists, columns, properties) => | |||
indexName, indexType, ifNotExists, columns, properties) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary change.
...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
Outdated
Show resolved
Hide resolved
…rces/v2/DataSourceV2Strategy.scala
Kubernetes integration test starting |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Kubernetes integration test status failure |
Test build #144987 has finished for PR 34486 at commit
|
thanks, merging to master! |
@huaxingao @cloud-fan Sorry, but I'll revert this change because of the GA failure, which seems to be related this change. |
sorry |
### What changes were proposed in this pull request? use property to specify index type ### Why are the changes needed? fix scala doc in #34486 and resubmit ### Does this PR introduce _any_ user-facing change? Yes ``` void createIndex(String indexName, String indexType, NamedReference[] columns, Map<NamedReference, Map<String, String>> columnsProperties, Map<String, String> properties) ``` changed to ``` createIndex(String indexName, NamedReference[] columns, Map<NamedReference, Map<String, String>> columnsProperties, Map<String, String> properties ``` ### How was this patch tested? new test Closes #34523 from huaxingao/newDelete. Authored-by: Huaxin Gao <huaxin_gao@apple.com> Signed-off-by: Kousuke Saruta <sarutak@oss.nttdata.com>
What changes were proposed in this pull request?
use property to specify index type
Why are the changes needed?
to address this comment #34148 (comment)
Does this PR introduce any user-facing change?
Yes
changed to
How was this patch tested?
new test