Fix issue 1222

[ericvergnaud]

Progresses #1222 by implementing DealiasLCAs, a transformation rule that replaces LCAs by the underlying alias expression in project filters (where clause of a select statement)

[github-actions]

Coverage tests results

462 tests  +6   424 ✅ +5   4s ⏱️ ±0s
  7 suites +1    38 💤 +1 
  7 files   +1     0 ❌ ±0 

Results for commit c247036. ± Comparison against base commit 9dcc986.

♻️ This comment has been updated with latest results.

[jimidle]

Find a way to not use asInstanceOf

[jimidle]

We should avoid isInstanceOf in favor of match

[ericvergnaud]

I'd agree if we cared about more than 1 type, but in this case it would make the code much more complex without a clear benefit ?

[jimidle]

The benefit is not using asInstanceOf ;)

[ericvergnaud]

how is that a clear benefit ?

[ericvergnaud]

( the style guide doesn't say anything on this topic..)

[vil1]

Suggested change

	.collect { case a: Alias => a }
	.filter(alias => !alias.child.isInstanceOf[Literal])
	.map(alias => alias.name.id -> alias.child)
	.collect { case Alias(e, name) if !e.isInstanceOf[Literal] => name.id -> e }

further distillation

[ericvergnaud]

done

[vil1]

The whole function could become:

Option(item).collect{
  case Name(name) => name
  case Id(id, _) => id
}
.flatMap(aliases.get)
.getOrElse(item)

or

val key = item match {
  case Name(name) => name
  case Id(id, _) => name
  case _ => null
}
aliases.getOrElse(key, item)

The former being slightly preferable, even though it is unlikely that aliases would have a entry with a null key.

[ericvergnaud]

outdated

[vil1]

I think the pattern of calling transform and then checking the result with eq against the original value before calling makeCopy if there was a change is a premature optimization that unnecessarily hurts readability. Even more so if we consider that transform already performs a similar optimization on the descendants of the transformed tree.

As an example, dealiasProject would be more readable as:

private def dealiasProject(project: Project, filter: Filter, aliases: Map[String, Expression]): Project = project.copy(input = dealiasFilter(filter, aliases))

at the expense of a single object allocation.

Moreover, as it relies on reflexion under the hood, makeCopy is prone to silently break if the order/types of constructor parameters gets changed in the future (silently = at runtime). If we insist on using makeCopy here, we should make ensure 100% test coverage to prevent such silent breaking from happening.

[vil1]

It wouldn't hurt to add a test where aliases appear deeply nested in an arbitrarily contrived expression involving boolean operators (including NOT), function calls, etc.

[ericvergnaud]

I think the pattern of calling transform and then checking the result with eq against the original value before calling makeCopy if there was a change is a premature optimization that unnecessarily hurts readability. Even more so if we consider that transform already performs a similar optimization on the descendants of the transformed tree.

As an example, dealiasProject would be more readable as:

private def dealiasProject(project: Project, filter: Filter, aliases: Map[String, Expression]): Project = project.copy(input = dealiasFilter(filter, aliases))

at the expense of a single object allocation.

Open for discussion, but the expense is much higher than a single object allocation since you'd scan and transform a lot of stuff before discovering that you haven't changed anything (and might have lost information along the way).

[vil1]

Open for discussion, but the expense is much higher than a single object allocation since you'd scan and transform a lot of stuff before discovering that you haven't changed anything (and might have lost information along the way).

If you look into transform implementation, you'll see that it already implements such "copy avoiding" mechanism, so wrapping the result of transform in a new node (ie, calling copy in our Rules) is a marginal cost (that doesn't justify hurting readability/future refactoring IMO).

[ericvergnaud]

Moreover, as it relies on reflexion under the hood, makeCopy is prone to silently break if the order/types of constructor parameters gets changed in the future (silently = at runtime). If we insist on using makeCopy here, we should make ensure 100% test coverage to prevent such silent breaking from happening.

I prefer makeCopy for now because as we've seen, the constructor doesn't convey origin, comments etc... so I fear information would be lost

[ericvergnaud]

Open for discussion, but the expense is much higher than a single object allocation since you'd scan and transform a lot of stuff before discovering that you haven't changed anything (and might have lost information along the way).

If you look into transform implementation, you'll see that it already implements such "copy avoiding" mechanism, so wrapping the result of transform in a new node (ie, calling copy in our Rules) is a marginal cost (that doesn't justify hurting readability/future refactoring IMO).

It would be interesting to measure that cost

[vil1]

I don't understand why we need overriding makeCopy here, is there a test somewhere showcasing the problem/solution ?

[ericvergnaud]

TreeNode.makeCopy docs say:

Must be overridden by child classes that have
   * constructor arguments that are not present in the productIterator.

I don't know exactly why only 1 arg of the Cast ctor is seen by the productIterator (maybe because they are not vals ?), but it's what happens.

This is tested via existing test cases in SnowflakeToDatabricksTranspilerTest, which crash without this bug fix.

[vil1]

This seems to be syntactically incorrect on Databricks (probably missing parenthesis around the subquery).
Also, the aliased name should be different from the columns they alias for, for the test to be really significant. (ca_zip in the WHERE clause could refer to customer_address.ca_zip, which isn't what we want)

[ericvergnaud]

The parenthesis issue is fixed in PR #1232.
This test is about homonyms, not subqueries, so I'm afraid I disagree with your comment re names.

[ericvergnaud]

Superseded by #1242