search

apache / datafusion-comet

Apache DataFusion Comet Spark Accelerator
https://datafusion.apache.org/comet
Apache License 2.0
447 stars 100 forks source link

chore: Use enum to represent CAST eval_mode in expr.proto #361

Open andygrove opened 2 weeks ago

andygrove commented 2 weeks ago

What is the problem the feature request solves?

When I added eval_mode to the Cast message in expr.proto I defined it as a string but it should really be an enum.

enum EvalMode {
  LEGACY = 0;
  TRY = 1;
  ANSI = 2;
}

Describe the potential solution

No response

Additional context

No response

prashantksharma commented 2 weeks ago

@andygrove

Thank you for the issue description.

I would like to work on this issue. If no one else is working on it. Can I take it up ?

viirya commented 2 weeks ago

Definitely yes. Thanks @prashantksharma

parthchandra commented 2 weeks ago

I suppose for this issue it is okay to violate the rule to not modify a field in a protobuf message because we haven't published anything yet.

andygrove commented 2 weeks ago

I suppose for this issue it is okay to violate the rule to not modify a field in a protobuf message because we haven't published anything yet.

Yes, absolutely.

prashantksharma commented 2 weeks ago

@andygrove , cc: @viirya

Minor Query before opening PR

Summary:

Screenshot 2024-05-04 at 16 15 58

Minor Query: Before opening the PR, I would like to confirm: if I should go ahead and resolve the error for planner.rs or a separate issue needs to be created for this ?

andygrove commented 1 week ago

On the Rust side you will need a match statement to convert the protobuf i32 to the Rust enum (0 -> legacy, 1 -> try, 2 -> ansi). Perhaps take a look at how we handle one of the other enums defined in the proto and follow the same pattern?

andygrove commented 1 week ago

@prashantksharma also, feel free to create a draft PR as it can be easier for maintainers to make suggestions on the PR

prashantksharma commented 1 week ago

@andygrove

Thank you so much for your feedback:

On the Rust side you will need a match statement to convert the protobuf i32 to the Rust enum (0 -> legacy, 1 -> try, 2 -> ansi). Perhaps take a look at how we handle one of the other enums defined in the proto and follow the same pattern?

I will make necessary changes based on my understanding of the above comment by you.

And, Thank you for your suggesting creation of draft PR, after making necessary updates, I will do that.

prashantksharma commented 1 week ago

@andygrove cc: @viirya

I have opended a draft PR.

I have tested the changes using

Details on PR message:https://github.com/apache/datafusion-comet/pull/415