Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Removed PartialOrd and Ord of all enums in datatypes #379

Merged
merged 1 commit into from
Sep 5, 2021

Conversation

jorgecarleitao
Copy link
Owner

@jorgecarleitao jorgecarleitao commented Sep 4, 2021

  • Removed PartialOrd nor Ord, since there is no natural order on them.
  • Made IntervalUnit and TimeUnit Copy, since they are simple enums.

@jorgecarleitao jorgecarleitao added the bug Something isn't working label Sep 4, 2021
@codecov
Copy link

codecov bot commented Sep 4, 2021

Codecov Report

Merging #379 (71b8968) into main (dbb7b8a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #379    +/-   ##
========================================
  Coverage   81.08%   81.09%            
========================================
  Files         328      330     +2     
  Lines       21748    21976   +228     
========================================
+ Hits        17635    17821   +186     
- Misses       4113     4155    +42     
Impacted Files Coverage Δ
src/datatypes/mod.rs 69.49% <ø> (ø)
src/compute/arithmetics/time.rs 97.48% <100.00%> (ø)
src/compute/cast/mod.rs 88.67% <100.00%> (ø)
src/compute/cast/primitive_to.rs 100.00% <100.00%> (ø)
src/temporal_conversions.rs 80.00% <100.00%> (ø)
src/io/json_integration/write.rs 0.00% <0.00%> (-6.25%) ⬇️
src/compute/filter.rs 71.35% <0.00%> (-4.03%) ⬇️
src/bitmap/utils/chunk_iterator/chunks_exact.rs 76.47% <0.00%> (-1.66%) ⬇️
src/io/json/read/reader.rs 74.32% <0.00%> (-1.36%) ⬇️
src/io/json_integration/schema.rs 44.58% <0.00%> (-0.31%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbb7b8a...71b8968. Read the comment docs.

@jorgecarleitao jorgecarleitao added backwards-incompatible and removed bug Something isn't working labels Sep 5, 2021
@jorgecarleitao jorgecarleitao merged commit 3af5ffe into main Sep 5, 2021
@jorgecarleitao jorgecarleitao deleted the clean_enums branch September 5, 2021 08:57
@jorgecarleitao jorgecarleitao changed the title Simplified enums in datatypes Removed PartialOrd and Ord of all enums in datatypes Sep 5, 2021
@yjshen
Copy link
Contributor

yjshen commented Dec 21, 2021

Hi @jorgecarleitao , arrow-datafusion added PartialOrd on Expr, which relays on DataType to be also PartialOrd:

#[derive(Clone, PartialEq, PartialOrd)]
pub enum Expr {
....
 Cast {
        /// The expression being cast
        expr: Box<Expr>,
        /// The `DataType` the expression will yield
        data_type: DataType,    // <- need DataType PartialOrd as well.
    },
}

Introduced in apache/datafusion#1014 to provide the capability of sorting expressions, for common-subexpression-elimination and further expression transformations. any thoughts on this?

@jorgecarleitao
Copy link
Owner Author

Ufff 😢

What is the semantics that the partial ord is trying to achieve in DataFusion?

I am a bit surprised that it is relying on the derived PartialOrd - semantically it seems to me that the order of the enum is meaningless. Is it to have a stable something?

@yjshen
Copy link
Contributor

yjshen commented Dec 22, 2021

Yes, I think so, for stabilizing tests (https://github.com/influxdata/influxdb_iox/pull/2564#discussion_r710289316) and expression algebraic transformations, as the issue brought this in explained apache/datafusion#1014

@houqp
Copy link
Collaborator

houqp commented Dec 22, 2021

yeah, i don't think the ordering semantic matters here, just need a consistent ordering defined.

@houqp
Copy link
Collaborator

houqp commented Dec 23, 2021

Pushed a workaround to implement partial ord using hash in datafusion: houqp/arrow-datafusion@99ee159.

@jorgecarleitao
Copy link
Owner Author

wow, quite neat idea! I had already given up and was dragging my feet to revert part of this PR and re-introduce partialOrd on

  • IntervalUnit
  • TimeUnit
  • Field
  • DataType

I am so glad that you found a way there!

@jorgecarleitao jorgecarleitao mentioned this pull request Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants