datafusion-comet icon indicating copy to clipboard operation
datafusion-comet copied to clipboard

feat: Use enum to represent CAST eval_mode in expr.proto

Open prashantksharma opened this issue 1 year ago • 0 comments

Which issue does this PR close?

Closes #361

Rationale for this change

The updates to expr.proto by using enums instead of strings, which prevents potential errors in type interpretation. Modifications in planner.rs and QueryPlanSerde.scala resolve issues found during compilation and style checks.

What changes are included in this PR?

  • expr.proto: Updated the eval_mode field from a string type to an enum type (EvalMode), with defined values LEGACY, TRY, and ANSI.
  • Rust Code: Modified ExprStruct::Cast(expr) in planner.rs to handle eval_mode as integers corresponding to the new protobuf enum. Updated the match statement to directly convert integers to the EvalMode Rust enum and added error handling for invalid integers.
  • QueryPlanSerde.scala:
    • Introduced the stringToEvalMode function, which converts input strings to ExprOuterClass.EvalMode enum values using a case-insensitive match, enhancing our error handling by throwing exceptions for invalid modes.
    • Corrected the use of toUpperCase to include Locale.ROOT to avoid locale-sensitive behavior, aligning with internationalization best practices.
    • Reorganized imports to comply with project's Scalastyle guidelines, clearly separating Java standard library imports from those specific to the project, which aids in code clarity and maintenance.
    • Addressed and resolved Scalastyle violations related to locale-specific string manipulations. Fixed the Scalastyle violation by correcting the usage of toUpperCase without specifying a locale. Reorganized import statements to adhere to the project's coding standards, ensuring that Java standard library imports are correctly grouped.

How are these changes tested?

I have run

  • make test-rust: No failures.
  • make test-jvm: No failures.

prashantksharma avatar May 11 '24 07:05 prashantksharma