chore: Refactor QueryPlanSerde to allow logic to be moved to individual classes per expression

[andygrove]

Which issue does this PR close?

Part of #1330

Rationale for this change

See #1330 for rationale.

What changes are included in this PR?

• Refactor QueryPlanSerde to move inner methods of exprToProto to top-level methods to make them accessible from other places
• Refactor ArrayRemove serde to demonstrate the benefit of this refactor

The ArrayRemove code in QueryPlanSerde now becomes:

case expr: ArrayRemove => CometArrayRemove.convert(expr, input, binding)

Later, we can move to a map of registered converters instead of a match statement for better extensibility. For example:

converters.get(expression.getClass).convert(expr, input, binding)

How are these changes tested?

There are no functional changes, so rely on existing tests.

[andygrove]

@erenavsarogullari @NoeB @dharanad @jatin510 please let me know if you have any feedback on this approach

[andygrove]

@parthchandra @mbutrovich fyi

[comphead]

thanks @andygrove. it definitely improves the access and testability for methods which have been inner

[parthchandra]

This is presumably not just for arrays, so perhaps should be in a common file?

[andygrove]

Yes, currently, there is only one implementation, which is why it is here at the moment, but this can move to a common class. Perhaps I should just move this into QueryPlanSerde source file

[andygrove]

I moved the trait and added some scaladocs.

[viirya]

Although there is a big diff, I suppose they are just code moving and this is the only change?

[andygrove]

Yes, there are no functional changes. I just moved the inner methods to the top-level and that required adding extra parameters in some cases (such as input and binding).

[kazuyukitanimura]

Any reason to switching to singular input from inputs?
The doc above still says inputs. Should we switch to plural inputs since it is Seq?

[andygrove]

In main, we currently have an inconsistency where exprToProto uses input and exprToProtoInternal uses inputs.

  def exprToProto(
      expr: Expression,
      input: Seq[Attribute],
      binding: Boolean = true): Option[Expr] = {

def exprToProtoInternal(expr: Expression, inputs: Seq[Attribute]): Option[Expr] = {

I changed both to input for consistency, but I agree that inputs is better. I have changed it.

[andygrove]

@kazuyukitanimura fyi, I fixed the above

[kazuyukitanimura]

Thanks @andygrove