Implement conversion from ColumnStatistics to NullableInterval #10510

demetribu · 2024-05-14T21:21:20Z

Which issue does this PR close?

This change addresses part of #10456.

Rationale for this change

What changes are included in this PR?

Added conversion from ColumnStatistics to NullableInterval to handle different nullability scenarios.
Defined Interval struct to represent the range of values in a column.
Implemented From for NullableInterval, handling Precision cases and determining nullability.
Added Unknown variant to NullableInterval to manage cases with insufficient statistics.

Are these changes tested?

Are there any user-facing changes?

jayzhan211 · 2024-05-15T00:10:35Z

datafusion/expr/src/interval_arithmetic.rs

-                    })
+                    match self.data_type() {
+                        Some(datatype) => Ok(Self::Null { datatype }),
+                        None => Err(DataFusionError::NotImplemented(


You can use not_impl_err! macro

jayzhan211

👍

berkaysynnada · 2024-05-15T07:20:44Z

datafusion/expr/src/interval_arithmetic.rs

@@ -1469,6 +1472,8 @@ pub enum NullableInterval {
    MaybeNull { values: Interval },
    /// The value is definitely not null, and is within the specified range.
    NotNull { values: Interval },
+    /// Added to handle cases with insufficient statistics
+    Unknown,
 }


MaybeNull with an unbounded interval corresponds to this Unknown variant, doesn't it ?

That's an interesting idea. Do you mean we should use ScalarValue::Null in Interval if the value is Absent?

For me, Interval::MaybeNull{Null, Null} looks strange, honestly.
@jayzhan211 wdyt?

Using ScalarValue::Null for representing unbounded intervals is discouraged. Although it's possible to construct these, performing arithmetic on them is not feasible. Could you detect the datatype and instead create ScalarValue::Datatype(None)? This approach is consistent with how we handle unbounded cases throughout the codebase.

What I saw is that sometimes ColumnStatistics { ..., min, max, ... } both are equal to Precision::Absent. In such cases, we can't determine the type for Interval.

When I tried to play with the stats in CLI mode, I attempted to get them from the ExecutionPlan. Here, there was a default absent value:

datafusion/datafusion/core/src/physical_planner.rs

Line 470 in c312ffe

let plan = self

I attempted to get them from the ExecutionPlan. Here, there was a default absent value:

I would expect that calling execution_plan.statistics() would match the schema returned from execution_plan.schema()

Will re-check tomorrow then

Anywhere we have ColumnStatistics we should also have the schema (and thus the DataType of all columns) -- maybe we just have to pass the schema along with the statistics?

Yes, I think that's the right approach for this issue. Can we open a ticket for this? It should be an easy fix, and ColumnStatistics isn't used in many parts of the code.

@alamb
I have conducted some additional tests on various queries and observed the following results:

CREATE TABLE data_table ( id INT, value INT );

INSERT INTO data_table (id, value) VALUES (1, 100), (2, 200), (3, 300), (4, 400), (5, 500), (6, 600), (7, 700), (8, 800), (9, 900), (10, 1000);

SELECT id, value FROM data_table WHERE value > 500;

Log:
Schema: id: Int32, value: Int32
Column 0: ColumnStatistics { null_count: Inexact(0), max_value: Exact(Int32(NULL)), min_value: Exact(Int32(NULL)), distinct_count: Absent }
Column 1: ColumnStatistics { null_count: Inexact(0), max_value: Inexact(Int32(NULL)), min_value: Inexact(Int32(501)), distinct_count: Absent }

SELECT AVG(value) AS average_value, SUM(value) AS total_value FROM data_table;

Log:
Schema: average_value: Float64, total_value: Int64
Column 0: ColumnStatistics { null_count: Absent, max_value: Absent, min_value: Absent, distinct_count: Absent }
Column 1: ColumnStatistics { null_count: Absent, max_value: Absent, min_value: Absent, distinct_count: Absent }

SELECT a.id AS id_a, a.value AS value_a, b.id AS id_b, b.value AS value_b FROM data_table a CROSS JOIN data_table b;

Log:
Schema: id_a: Int32, value_a: Int32, id_b: Int32, value_b: Int32
Column 0: ColumnStatistics { null_count: Exact(0), max_value: Absent, min_value: Absent, distinct_count: Absent }
Column 1: ColumnStatistics { null_count: Exact(0), max_value: Absent, min_value: Absent, distinct_count: Absent }
Column 2: ColumnStatistics { null_count: Exact(0), max_value: Absent, min_value: Absent, distinct_count: Absent }
Column 3: ColumnStatistics { null_count: Exact(0), max_value: Absent, min_value: Absent, distinct_count: Absent }

alamb

Thanks @dmitrybugakov @jayzhan211 and @berkaysynnada

alamb · 2024-05-15T19:01:24Z

datafusion/expr/src/interval_arithmetic.rs

@@ -1469,6 +1472,8 @@ pub enum NullableInterval {
    MaybeNull { values: Interval },
    /// The value is definitely not null, and is within the specified range.
    NotNull { values: Interval },
+    /// Added to handle cases with insufficient statistics


Suggested change

/// Added to handle cases with insufficient statistics

/// No information is known about this intervals values or nullness

alamb · 2024-05-15T19:03:53Z

datafusion/expr/src/interval_arithmetic.rs

@@ -1469,6 +1472,8 @@ pub enum NullableInterval {
    MaybeNull { values: Interval },
    /// The value is definitely not null, and is within the specified range.
    NotNull { values: Interval },
+    /// Added to handle cases with insufficient statistics
+    Unknown,
 }


What I saw is that sometimes ColumnStatistics { ..., min, max, ... } both are equal to Precision::Absent. In such cases, we can't determine the type for Interval.

Anywhere we have ColumnStatistics we should also have the schema (and thus the DataType of all columns) -- maybe we just have to pass the schema along with the statistics?

This change addresses part of apache#10456.

demetribu · 2024-05-16T16:03:02Z

@alamb
Do we want to introduce a Type in ColumnStats in this PR? I found that the changes will affect Proto and may require other changes that are not clear for me at the moment.

datafusion/datafusion/core/src/datasource/statistics.rs

Line 141 in 842f393

column_statistics: get_col_stats_vec(null_counts, max_values, min_values),

datafusion/datafusion/core/src/datasource/physical_plan/file_scan_config.rs

Line 124 in 842f393

if idx < self.file_schema.fields().len() {

Or should I close the PR and create a new issue instead?

github-actions bot added the logical-expr Logical plan and expressions label May 14, 2024

demetribu force-pushed the convert-stats-to-interval-10456 branch from dd9f55f to 0d75e43 Compare May 14, 2024 21:22

jayzhan211 reviewed May 15, 2024

View reviewed changes

jayzhan211 approved these changes May 15, 2024

View reviewed changes

demetribu force-pushed the convert-stats-to-interval-10456 branch from 0d75e43 to 9ff8cb4 Compare May 15, 2024 07:15

berkaysynnada reviewed May 15, 2024

View reviewed changes

alamb reviewed May 15, 2024

View reviewed changes

Implement conversion from ColumnStatistics to NullableInterval

b745673

This change addresses part of apache#10456.

demetribu force-pushed the convert-stats-to-interval-10456 branch from 9ff8cb4 to b745673 Compare May 15, 2024 19:10

demetribu closed this May 17, 2024

demetribu deleted the convert-stats-to-interval-10456 branch May 17, 2024 08:57

jayzhan211 mentioned this pull request Jun 9, 2024

Apply guarantee rewriter to sql workflow #10456

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement conversion from ColumnStatistics to NullableInterval #10510

Implement conversion from ColumnStatistics to NullableInterval #10510

demetribu commented May 14, 2024

jayzhan211 May 15, 2024

jayzhan211 left a comment

berkaysynnada May 15, 2024

demetribu May 15, 2024 •

edited

Loading

demetribu May 15, 2024

berkaysynnada May 15, 2024

demetribu May 15, 2024

demetribu May 15, 2024 •

edited

Loading

alamb May 15, 2024

demetribu May 15, 2024

berkaysynnada May 16, 2024

demetribu May 16, 2024

alamb left a comment

alamb May 15, 2024

alamb May 15, 2024

demetribu commented May 16, 2024

	/// Added to handle cases with insufficient statistics
	/// No information is known about this intervals values or nullness

Implement conversion from ColumnStatistics to NullableInterval #10510

Implement conversion from ColumnStatistics to NullableInterval #10510

Conversation

demetribu commented May 14, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

jayzhan211 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demetribu May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demetribu May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

demetribu commented May 16, 2024

demetribu May 15, 2024 •

edited

Loading

demetribu May 15, 2024 •

edited

Loading