[SPARK-9182][SQL]Filters are not passed through to jdbc source

[yjshen]

This PR fixes unable to push filter down to JDBC source caused by Cast during pattern matching.

While we are comparing columns of different type, there's a big chance we need a cast on the column, therefore not match the pattern directly on Attribute and would fail to push down.

[yjshen]

/cc @liancheng

[SparkQA]

Test build #40230 has finished for PR 8049 at commit 890e66c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #40279 has finished for PR 8049 at commit e186ca2.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[liancheng]

I haven't successfully constructed a test case to prove it, but simply removing the casts here looks a little bit dangerous to me. For example, both BinaryOperator and logic in ParquetFilters assume that both branches of a binary expression should have the same data type.

Instead of removing the casts, how about transposing them? Namely, converting

expression.transform {
  case LessThan(Cast(a: Attribute, _), value) => 
    LessThan(a, Cast(value, a.dataType).eval())

  case LessThan(value, Cast(a: Attribute, _)) => 
    LessThan(Cast(value, a.dataType).eval(), a)

  ...
}

In this way, we still ensure both branches have the same data type.

[yjshen]

Thanks, I didn't notice the assumption of parquetFilters until you pointed out. I'll follow your advice above.

[yjshen]

when we are comparing DateType column with a String literal, we could not cast the String literal value to DateType since it would eval to the inner representation of DateType, i.e. int, which means nothing for JDBC source.

[liancheng]

Cast.eval() returns values of Catalyst internal types, and can be converted to values of user space types via CatalystTypeConverter.convertToScala(). This is not super efficient, but we are not on the critical path, so I think it's OK.

[yjshen]

Jenkins, retest this please.

[yjshen]

some problem with Jenkins?

[yjshen]

Jenkins, retest this please.

[SparkQA]

Test build #40393 has finished for PR 8049 at commit 4977fe7.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[yjshen]

Jenkins, retest this please.

[SparkQA]

Test build #40403 has finished for PR 8049 at commit 43380c3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #40458 has finished for PR 8049 at commit cebdd1d.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[liancheng]

@marmbrus Well, seems that GitHub ate your comment about Literal extractor, so I'm quoting it here:

It would be nice if we could just build the convertToScala logic into the extractor for Literal.

We can have separate extractors like InternalLiteral for Catalyst values and Literal extractor or Scala values.

@yjshen This can be done in separate PR though.

[liancheng]

@yjshen Thanks for fixing this! Merging to master and branch-1.5.

[marmbrus]

is this always safe? or could you for example cast long -> int and truncate?

[yjshen]

yes, you are right, downcast here just truncate the origin literal value and have a wrong pushed down filter.

[marmbrus]

so should we revert this?

/cc @liancheng

[yjshen]

Hi, @marmbrus , I've think this twice and may be I could do this?

For HadoopFsRelations, they all assume pushed down column and value are of same type, I think the only safe way is not pushed down these casted filters at all.
For JDBCRelation, since the value itself is converted into a constructed string where clause and pushed to the underlying database, I think it's safe to just pushed to uncasted value down?

[marmbrus]

Hmm, depends on what implicit casting rules various databases do. Can you investigate more? I would not want us to generate queries that fail to analayze.

In the mean time I think we should revert this from the release branch as pushing down wrong filters is worse than not pushing down filters.

[yjshen]

ok, sorry for the wrong fix, should I make a revert PR?

[marmbrus]

Its okay. I'm glad we caught it. Please do and open a blocker JIRA targeted at 1.5 so we don't miss merging it.

[liancheng]

It's reverted by #8157. I think we can add casts over literals only when the casts don't introduce truncation?

I reopened SPARK-9182. It's not a regression introduced in 1.5, do we need a blocker JIRA for it?

[marmbrus]

No, given the possible trickiness here I think we should bump the fix to 1.6.