Introduce SourceRelation/FileBasedRelation traits to remove direct dependency on LogicalRelation from actions/rules #355

imback82 · 2021-02-11T03:18:55Z

What is the context for this pull request?

#321 and #320 require to support DatasourceV2Relation, but the current provider was designed to support only the LogicalRelation, so adding a new provider that supports a different relation type requires lots of code changes across actions/rules.

What changes were proposed in this pull request?

This proposes to introduce one more abstraction to FileBasedSourceProvider such that each provider now needs to implement FileBasedRelation. For example, DeltaLakeFileBasedSource needs to implement DeltaRelation which extends FileBasedRelation to handle a Delta Lake specific relation.

By decoupling this, actions/rules do not depend on LogicalRelation directly.

Does this PR introduce any user-facing change?

No

How was this patch tested?

API refactoring. Existing tests should be enough.

andrei-ionescu · 2021-02-11T10:19:42Z

@imback82 Just some early feedback...

I've seen you added the FileBasedRelation trait. The DataSourceV2Relation is not necessarily a file base relation. I would suggest to have this trait defined as a more generic relation, not only file based ones. As you suggest on my #321 PR SourceRelation fits better.

imback82 · 2021-02-11T17:01:47Z

We need to have files-related APIs such as allFiles at FileBasedRelation (I thought about having SupportsFiles trait, but didn't want to complicate APIs at this stage). Until we have a good use case/example of supporting non-filed relations, making it explicit seems reasonable (similar to we store FileBasedSourceProviderManager in the hyperpsace context).

Similar to SourceProvider, I can have a top level trait SourceRelation, but will still use FileBasedRelation in FileBasedSourceProviderManager.getRelation.

andrei-ionescu · 2021-02-11T18:16:26Z

First, regarding Iceberg and DataSourceV2...

The Iceberg source does not use and index (like the InMemoryFileIndex) and the files attached to an Iceberg table are not attached to a relation. To retrieve the files of an Iceberg table you need to use the table file scan api.

See https://github.com/microsoft/hyperspace/pull/320/files#diff-01b72d2de6f62e2696e47b850eb7e40039fa85120d40ef392df1cf7b44aff9f8R246-R259

Second, it does NOT look right to have the DataSourceV2Relation hard linked to the FileBasedRelation and I don't know if this refactor make the code clearer in the case of DataSourceV2 sources like Iceberg.

In the end, if you still consider that this will bring clarity, please complete this PR ASAP and merge it, so that I could modify my PRs to fit these changes.

Thank you.

imback82 · 2021-02-11T18:21:00Z

Second, it does NOT look right to have the DataSourceV2Relation hard linked to the FileBasedRelation and I don't know if this refactor make the code clearer in the case of DataSourceV2 sources like Iceberg.

Why not? I see that you are implementing allFiles? Your provider for iceberg can just check if the given datasourcev2relation is applicable.

I will get this ready for review by eod today as promised.

imback82 · 2021-02-12T01:30:52Z

src/main/scala/com/microsoft/hyperspace/actions/CreateActionBase.scala

    }
  }

-  protected def sourceRelations(spark: SparkSession, df: DataFrame): Seq[Relation] =


This is no longer needed as creating Relation has moved to FileBasedRelation.

imback82 · 2021-02-12T01:31:33Z

src/main/scala/com/microsoft/hyperspace/actions/CreateActionBase.scala

    (indexDF, resolvedIndexedColumns, resolvedIncludedColumns)
  }
-
-  private def getPartitionColumns(df: DataFrame): Seq[String] = {


This is no longer needed as FileBasedRelation supports getting the partition schema.

imback82 · 2021-02-12T01:32:09Z

src/main/scala/com/microsoft/hyperspace/actions/RefreshActionBase.scala

-        Hyperspace
-          .getContext(spark)
-          .sourceProviderManager
-          .allFiles(relation)


allFiles has moved to FileBasedRelation.

imback82 · 2021-02-12T01:36:07Z

src/main/scala/com/microsoft/hyperspace/index/rules/JoinIndexRule.scala

-    RuleUtils.getLogicalRelation(l).isDefined && RuleUtils.getLogicalRelation(r).isDefined &&
-    isPlanLinear(l) && isPlanLinear(r) && !isPlanModified(l) && !isPlanModified(r) &&
-    ensureAttributeRequirements(l, r, condition)
+    isPlanLinear(l) && isPlanLinear(r) &&


@apoorvedave1 we don't need isPlanLinear if getRelation implementation is as follows, right (collecting leaves and making sure it's only one)?:

def getRelation(spark: SparkSession, plan: LogicalPlan): Option[FileBasedRelation] = { val provider = Hyperspace.getContext(spark).sourceProviderManager val leaves = plan.collectLeaves() if (leaves.size == 1 && provider.isSupportedRelation(leaves.head)) { Some(provider.getRelation(leaves.head)) } else { None } }

Yeah that makes sense, we can eliminate isPlanLinear check

imback82 · 2021-02-12T01:37:01Z

src/main/scala/com/microsoft/hyperspace/index/rules/JoinIndexRule.scala

-   * @param plan Logical plan.
-   * @return true if the relation in the plan is modified by Hyperspace.
-   */
-  private def isPlanModified(plan: LogicalPlan): Boolean = {


removed because this is directly testes in isApplicable above.

imback82 · 2021-02-12T01:38:43Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

-          Hyperspace
-            .getContext(spark)
-            .sourceProviderManager
-            .allFiles(rel)


again, allFiles has moved to FileBasedRelation (see below).

imback82 · 2021-02-12T01:47:37Z

src/main/scala/com/microsoft/hyperspace/index/rules/RuleUtils.scala

-          .partitionBasePath(location)
+      case l: LeafNode if provider.isSupportedRelation(l) =>
+        val relation = provider.getRelation(l)
+        val options = relation.partitionBasePath


@sezruby Question. Below line 482, do we need to create a tag on the originalPlan or can it be just the logical plan for the relation?:

val newLocation = index.withCachedTag( originalPlan, IndexLogEntryTags.INMEMORYFILEINDEX_HYBRID_SCAN_APPENDED) { new InMemoryFileIndex(spark, filesAppended, options, None) }

getCandidateIndexes uses the relation plan, so I was curious.

Yea getCandidateIndexes using a relation plan, but for here I used originalPlan.
It might be better to use the relation plan here as it's more reusable.

thanks, we can do that as a follow up (wanted to minimize the behavior change in this PR)

imback82 · 2021-02-12T01:48:11Z

src/main/scala/com/microsoft/hyperspace/index/sources/FileBasedSourceProviderManager.scala

   */
-  def refreshRelation(relation: Relation): Relation = {
-    run(p => p.refreshRelation(relation))
+  def refreshRelationMetadata(relation: Relation): Relation = {


We have so many "Relation"s, so I am explicitly calling it out as relation metadata here now.

imback82 · 2021-02-12T01:50:25Z

src/main/scala/com/microsoft/hyperspace/index/sources/default/DefaultFileBasedRelation.scala

+/**
+ * Implementation for file-based relation used by [[DefaultFileBasedSource]]
+ */
+class DefaultFileBasedRelation(spark: SparkSession, override val plan: LogicalRelation)


Most of the implementation is from DefaultFileBasedSource with minor changes to change the return type: we don't need to wrap with Option any longer.

imback82 · 2021-02-12T01:51:07Z

src/main/scala/com/microsoft/hyperspace/index/sources/default/DefaultFileBasedSource.scala

      formats.toLowerCase(Locale.ROOT).split(",").map(_.trim).toSet
    })

-  /**


Removed code has moved to DefaultFileBasedRelation.

imback82 · 2021-02-12T01:55:56Z

@andrei-ionescu @sezruby @apoorvedave1 This is ready for review now (I will update the PR description soon, but I believe you already know the context. 😄 ). Basically, I removed the direct dependency on LogicalRelation from actions/rules code.

@andrei-ionescu Now, you can just implement IcebergProvider and IcebergRelation without modifying anywhere else, hopefully, 🤞🏼

This reverts commit 22f017a.

we worked on this together Co-Authored-By: Andrei Ionescu <webdev.andrei@gmail.com>

imback82 · 2021-02-12T02:10:58Z

@andrei-ionescu I also add you as a co-author in this commit 50ed5cd, so when this PR is merged, you will get the co-authorship. Thanks!

andrei-ionescu · 2021-02-12T16:27:20Z

@imback82 Thanks for co-authoring!

sezruby · 2021-02-12T21:06:08Z

The approach looks great. Thanks :)

apoorvedave1

LGTM, 👍 thanks @imback82

imback82

Thanks @andrei-ionescu @sezruby @apoorvedave1 for the review!

I will create a follow up PR to address a couple of comments.

initial commit

d2e67ba

imback82 self-assigned this Feb 11, 2021

imback82 added the enhancement New feature or request label Feb 11, 2021

imback82 added this to the February 2021 (v0.5.0) milestone Feb 11, 2021

imback82 added 2 commits February 10, 2021 20:36

Replace getLogicalRelation with getRelation

9b66b4c

remove LogicalRelation usage from JoinIndexRule.scala

594c1bc

imback82 added 9 commits February 11, 2021 10:21

getCandidateIndexes cleanup

c7030c1

partitionSchema and toLogicalPlan in FileSourceRelation

46e8dcd

createHadoopFsRelation and createLogicalRelation

b7ae1cd

Remove dependency on LogicalRelation from RuleUtils.scala.

e35e57a

Remove unused APIs, general clean up

15b0ec8

Remove direct dependency on LogicalRelation from CreateActionBase.scala

3f7141e

Remove LogicalRelation dependency from RefreshActionBase.scala

a038b8a

Remove dependency on LogicalRelation from CreateAction.scala

1afdbec

restructure files

76b6388

imback82 changed the title ~~[WIP] Introduce FileBasedRelation trait to decouple relation types from source provider.~~ Introduce Feb 12, 2021

imback82 changed the title ~~Introduce~~ Introduce SourceRelation/FileBasedRelation traits to remove direct dependency on LogicalRelation from actions/rules Feb 12, 2021

imback82 commented Feb 12, 2021

View reviewed changes

imback82 and others added 3 commits February 11, 2021 18:00

fix typo

22f017a

Revert "fix typo"

356b6f4

This reverts commit 22f017a.

Add co-author

50ed5cd

we worked on this together Co-Authored-By: Andrei Ionescu <webdev.andrei@gmail.com>

andrei-ionescu approved these changes Feb 12, 2021

View reviewed changes

sezruby approved these changes Feb 12, 2021

View reviewed changes

apoorvedave1 approved these changes Feb 12, 2021

View reviewed changes

imback82 commented Feb 12, 2021

View reviewed changes

imback82 merged commit 0fef997 into microsoft:master Feb 12, 2021

This was referenced Feb 15, 2021

Support Iceberg table format #358

Merged

Support Iceberg table format #320

Closed

Support DataSourceV2 sources #321

Closed

Introduce SourceRelation/FileBasedRelation traits to remove direct dependency on LogicalRelation from actions/rules #355

Introduce SourceRelation/FileBasedRelation traits to remove direct dependency on LogicalRelation from actions/rules #355

Uh oh!

Conversation

imback82 commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the context for this pull request?

What changes were proposed in this pull request?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

andrei-ionescu commented Feb 11, 2021

Uh oh!

imback82 commented Feb 11, 2021

Uh oh!

andrei-ionescu commented Feb 11, 2021

Uh oh!

imback82 commented Feb 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoorvedave1 Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 commented Feb 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imback82 commented Feb 12, 2021

Uh oh!

andrei-ionescu commented Feb 12, 2021

Uh oh!

sezruby commented Feb 12, 2021

Uh oh!

apoorvedave1 left a comment

Choose a reason for hiding this comment

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imback82 commented Feb 11, 2021 •

edited

Loading

apoorvedave1 Feb 12, 2021 •

edited

Loading

imback82 commented Feb 12, 2021 •

edited

Loading