Unsupported binary operator `StringConcat`

[andygrove]

Describe the bug

Could not parse logical plan protobuf: Error during planning: General error: Unsupported binary operator '\\\"StringConcat\\\"

To Reproduce Steps to reproduce the behavior:

Expected behavior A clear and concise description of what you expected to happen.

Additional context Add any other context about the problem here.

[askoa]

I think a possible fix is to include a function like from_str in arrow-datafustion#Operator and use it in arrow-ballista. I see that ballista's code has not included Bitwise operators too. This issue will resurface whenever a new binary operator is included in datafusion project. Including the from_str function will ensure two projects need not be modified when a binary operator is included.

[askoa]

@andygrove Sorry for being little late for the party. My impression of the ticket was that the error quoted in the issue description is generated in the function from_proto_binary_op. To fix the issue, StringConcat has to be added to the match op block in that function.

I also see there might be more issues in future, now that Bitwise operators are included in the Operator which is not yet added to the match op block. I wrote an earlier comment that rather than keep updating the match op block in arrow-ballista project, its better to include a from_str function in the arrow-datafusion project.

I see that the issue is closed without updating match op block. It would be great to know how the issue is fixed without updating the match op block. Could you please explain?

[andygrove]

Hi @askoa. The bug that I fixed was related to logical plan serialization. This is part of the datafusion-proto crate that Ballista uses. You are right though .. we also need to fix this in the physical plan serialization.

[andygrove]

@askoa, I created a PR to add the missing operators, but I agree with your comments that we could leverage DataFusion rather than have duplicate code here. DataFusion has a from_proto_binary_op that we could use, but it is currently private.

We also need some tests in Ballista around this.

[askoa]

@andygrove Thanks for the response. I'll pick this one up.