Github user dongjoon-hyun commented on the issue:
https://github.com/apache/spark/pull/14235
It's definitely a solution. In fact, I tried that first before making this
PR, of course, not completely, but as a feasibility tests. It has its own pros
and cons, too.
For example, if we add new optimizers like `OptimizeIn` by removing
duplications, we should change here too. (If there is a case here.) This is the
same situation because we didn't check the any plans or queries until now. We
only check the parseability and the result.
I'm sure that all attempts beyond the existing suite have the same
side-effects.
This PR aims to ensure the correctness before making the changes on
SQLBuilder and Hint, or other refactoring. It has its meaning since the current
testsuite is fragile as we know.
You may want to change the level of importance or test coverage for any
convenience. I understand the intention of @gatorsmile , too.
But, IMHO, it's difficult to refactor some module without a proper
testsuite. At the worst case, your can remove the second parameters easily when
we reach more stable level. It's similar with changing `test()` into `ignore()`.
Lastly, let me suggest like this. My opinion is having this now for further
refactoring. After all SQL functions moves into each LogicalPlans unittests, we
can downgrade this to compare optimized plan in most cases (not all cases).
How do you think about this way, @rxin and @gatorsmile ?
---
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]
