Github user culler commented on the pull request:
https://github.com/apache/spark/pull/3158#issuecomment-62395022
â
> Hey @culler, I noticed that a significant part of the this PR is about
changing a === b to
> EQ(a).===(b). I understand that this is used to avoid function name
collision between
> ScalaTest === and the newly introduced === DSL operator for Date and
Timestamp,
âYes, I am very unhappy about that. âI will study further how to work
around it some other way.
However, you have not understood the problem. The problem does not have
anything to do with my tests. Even if I completely remove my tests, the
problem will still arise. Also, the changes I am proposing do not change the
DSL at all. The ugly `EQ` construction *only* appears in scala source files
which import *both* the scalatest FunSuite *and* the SQLContext conversions.
That ugly stuff *never* appears in a DSL expression.
Let me try to explain. I have added some new implicit conversions. When
these conversions are in scope they add `===` operators to some basic types,
e.g Int. The definition of this `===` operator is:
`def === (other: Symbol) = EqualTo(literal, other)`
so this implicit conversion would not be applied when evaluating
expressions such as the ones in test files which import scalatest. E.g. it
does not apply in
`assert(literals.size === 4)`
because the right hand side (4) is not a Symbol. So there really is no
conflict here.
The problem is that even though the implicit conversion does not apply, the
scalatest `assert` **macro** is nonetheless deciding that the `literals.size`
already has an `===` operator, and so it refuses to apply its own implicit
conversion which would add a different `===` operator.
Note that this situation is different from the one which arises with the
DSL `===` operator. In that case the operator is simply a method of
Expression. If a DSL expression begins with a Symbol the implict conversions
convert the Symbol to an Expression which then has an `===` operator. But this
does not work when the DSL expression begins with a literal. In fact, this is
exactly the reason why the DSL could not recognize and expression like `0 <
'x`. This is the problem I am solving.
> EQ(a).===(b) looks too cumbersome, and looses all the meaning about DSL
operators --
> conciseness and readability.
âPlease note, this construction would never be used in a DSL
expression.â
> The === name collision problem had already been solved once when
> org.apache.spark.sql.catalyst.dsl.=== is introduced, and it has been
proven to be easy to
> workaround without such significant change.
The situation was different, since that `===` operator is only a method of
Expression.â
> So my suggestion is that, to workaround this issue. You may add your
tests in DslQuerySuite
> with the help of checkAnswer, and simply don't use both === within a same
scope.
âUnderstood. But that will not solve the problem. The other tests will
still fail to compile.â
Given that the `EQ` construction is not acceptable for use in a test, I
will look into the possibility of requiring a separate import for these "left
hand side" conversions, so that they would not need to be in scope for any of
the tests that are not testing this specific feature. Maybe that is the
cleanest workaround.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
