[GLUTEN-5569][VL] Hide child WriteFilesExec from VeloxColumnarWriteFilesExec on UI

[zhztheplayer]

Fixes #5569

Before:

After:

[github-actions]

#5569

[ulysses-you]

Please, do not make things complex... I strongly suggest revert previous pr and just inherit case class for now.

[zhztheplayer]

Please, do not make things complex... I strongly suggest revert previous pr and just inherit case class for now.

I don't agree. Please let me know if any Scala project with good code quality has case-class inheritance.

To me doing case-class inheritance makes thing complex. I know Scala doesn't do well for extensibility of case-class, I don't like this limitation either. But if it's the principle of Scala, I'd follow it.

[JkSelf]

@zhztheplayer @ulysses-you
Thank you very much for the optimization and suggestions. Indeed, extending a case class in Scala is not considered good practice. However, introducing such significant changes for this fix could complicate future code maintenance. I believe the root of the issue lies with vanilla Spark, and there should be an abstraction of the WriteFilesExec class to facilitate extension. I see that a similar abstraction has already been done with BaseAggregateExec in the current vanilla Spark. Could we possibly submit a PR to the Spark community to address this issue?

[zhztheplayer]

I believe @ulysses-you has submitted this. I knew that should definitely help on this issue however the old Spark versions will take long time to leave from Gluten. So any "temporary" workaround on the issue could become somewhat long-term solution in Gluten. That's a reason why I think we should not rely on workarounds...

[ulysses-you]

My point is that if there is an actually issue with case class, I'm fine to change it since it is a fix, otherwise just add a todo and leave it.

I searched Spark code repo, and really find a case... is it valid for you @zhztheplayer ?

Yes, Spark master branch has a trait for this but older version did not.

[zhztheplayer]

I searched Spark code repo, and really find a case... is it valid for you @zhztheplayer ?

The case is not really case-class inheritance we are discussing here. It just creates an alias from AlwaysTrue$ to val INSTANCE = new AlwaysTrue(). Case-class's default members, say equals, hashCode, apply, unapply are still valid and correct.

Having said that I am personally not into the approach either. It's better to be AlwaysTrue.INSTANCE or something.

My point is that if there is an actually issue with case class

Once we get some, it could be very hard to trouble-shoot. Say Spark's subquery / exchange reuse rely on case-class equality, and test cases could rely on case-class's unapply to check plan.

I remember we used to suffer from one or several similar case-class inheritance issues from very early stage of Gluten (probably Gazelle, I can't recall it), and the issues really costed us effort to debug and fix. If we don't keep cleaning similar code, I highly doubt we will soon face another issue again.