[Docs] inconsistency between C++ implementation and the statistics schema

[arashandishgar]

Describe the bug, including details regarding any error messages, version, and platform.

I have implemented the Example in c++ and as it is mentioned in this link I should expect a physical layout which has 9 elements in colunmn_field but I got only three elements in column_field

main.zip

Component(s)

Documentation

[kou]

Good catch!

The example was wrong. column should not have duplicated values.

[arashandishgar]

I think there is another issue in Record Bath statistics

arrow/cpp/src/arrow/record_batch.h

Line 289 in ec3d283

/// https://arrow.apache.org/docs/format/CDataInterfaceStatistics.html

the link does not exist no more

[arashandishgar]

There is another issue in a test regarding Record batch statistics.

arrow/cpp/src/arrow/record_batch_test.cc

Lines 1395 to 1424 in ec3d283

	TEST_F(TestRecordBatch, MakeStatisticsArrayMaxApproximate) {
	auto schema =
	::arrow::schema({field("no-statistics", boolean()), field("float64", float64())});
	auto no_statistics_array = ArrayFromJSON(boolean(), "[true, false, true]");
	auto float64_array_data = ArrayFromJSON(float64(), "[1.0, null, -1.0]")->data()->Copy();
	float64_array_data->statistics = std::make_shared<ArrayStatistics>();
	float64_array_data->statistics->min = -1.0;
	auto float64_array = MakeArray(std::move(float64_array_data));
	auto batch = RecordBatch::Make(schema, float64_array->length(),
	{no_statistics_array, float64_array});
	ASSERT_OK_AND_ASSIGN(auto statistics_array, batch->MakeStatisticsArray());
	ASSERT_OK_AND_ASSIGN(
	auto expected_statistics_array,
	MakeStatisticsArray("[null, 1]",
	{{
	ARROW_STATISTICS_KEY_ROW_COUNT_EXACT,
	},
	{
	ARROW_STATISTICS_KEY_MIN_VALUE_APPROXIMATE,
	}},
	{{
	ArrayStatistics::ValueType{int64_t{3}},
	},
	{
	ArrayStatistics::ValueType{-1.0},
	}}));
	AssertArraysEqual(*expected_statistics_array, *statistics_array, true);
	}

This test is about min approximate value but title is about max approximate value

[arashandishgar]

I think one thing that is not clear about schema statistics specification is what I should expect from nested type statistics. I know that nested type statistics have not been implemented yet. link but nested types can have statistics like a simple type. Should the implementation accept all of that? for example is it acceptable that a struct in record batch to have max_approximate?

[kou]

There is another issue in a test regarding Record batch statistics.

This test is about min approximate value but title is about max approximate value

Good catch! Could you open a separated issue for this?

Let's fix it in a separated PR.

[kou]

Should the implementation accept all of that? for example is it acceptable that a struct in record batch to have max_approximate?

Yes.

We may need to add arrow::Scalar or something to arrow::ArrayStatistics::ValueType for it. (Or we may use arrow::Scalar instead of arrow::ArrayStatistics::ValueType.)

[arashandishgar]

let imagine a type like
auto type = struct_({field("list_a", fixed_size_list(int64(), 2)), field("list_b", fixed_size_list(int64(), 2))});
I think even if there is a max element for the struct(not child type) it would probably be a two fixed_size_list again. Thus it could not be a scalar

[arashandishgar]

There is another issue in a test regarding Record batch statistics.
This test is about min approximate value but title is about max approximate value

Good catch! Could you open a separated issue for this?

Let's fix it in a separated PR.

I think things connected to C++ implementation would likely better be solved in this issue. I will probably pick it up after I finish another issue.(One issue has already been assigned to me.)

[kou]

let imagine a type like auto type = struct_({field("list_a", fixed_size_list(int64(), 2)), field("list_b", fixed_size_list(int64(), 2))}); I think even if there is a max element for the struct(not child type) it would probably be a two fixed_size_list again. Thus it could not be a scalar

We have StructScalar and FixedSizeListScalar:

arrow/cpp/src/arrow/scalar.h

Lines 714 to 727 in c7a9100

	struct ARROW_EXPORT StructScalar : public Scalar {
	using TypeClass = StructType;
	using ValueType = std::vector<std::shared_ptr<Scalar>>;
	ScalarVector value;
	Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
	StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
	: Scalar(std::move(type), is_valid), value(std::move(value)) {}
	static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
	std::vector<std::string> field_names);
	};

arrow/cpp/src/arrow/scalar.h

Lines 705 to 712 in c7a9100

	struct ARROW_EXPORT FixedSizeListScalar : public BaseListScalar {
	using TypeClass = FixedSizeListType;
	FixedSizeListScalar(std::shared_ptr<Array> value, std::shared_ptr<DataType> type,
	bool is_valid = true);
	explicit FixedSizeListScalar(std::shared_ptr<Array> value, bool is_valid = true);
	};

It seems that they can represet the max value.

[kou]

I think things connected to C++ implementation would likely better be solved in this issue. I will probably pick it up after I finish another issue.(One issue has already been assigned to me.)

The issue is for enhancing the current feature. But the test isn't an enhancement. It's a bug fix. I think that we should use a separated issue/pull request.

I think that we can merge the latter soon because the fix will be straightforward.

If we mix them, we will not be able to merge soon. Because the former will be complex. We can't merge only the latter.

[arashandishgar]

I think things connected to C++ implementation would likely better be solved in this issue. I will probably pick it up after I finish another issue.(One issue has already been assigned to me.)

The issue is for enhancing the current feature. But the test isn't an enhancement. It's a bug fix. I think that we should use a separated issue/pull request.

I think that we can merge the latter soon because the fix will be straightforward.

If we mix them, we will not be able to merge soon. Because the former will be complex. We can't merge only the latter.

Okay I will do it

by the way what about this?

I think there is another issue in Record Bath statistics

arrow/cpp/src/arrow/record_batch.h

Line 289 in ec3d283

/// https://arrow.apache.org/docs/format/CDataInterfaceStatistics.html

the link does not exist no more

[kou]

Ah, let's work on it as another separated task too. We'll be able to merge it quickly.

[arashandishgar]

Ah, let's work on it as another separated task too. We'll be able to merge it quickly.

Okay I will do it

[kou]

Issue resolved by pull request 45561
#45561

[arashandishgar]

Issue resolved by pull request 45561 #45561

several things are remained which has not been addressed yet.
1-lack implementation for nested type as only bool,int,float,string are supported in ArrayStatistics
2- several statistics attributed has not implemented yet such as "ARROW:row_count:approximate","ARROW:average_byte_width:exact","ARROW:average_byte_width:approximate"

[kou]

1-lack implementation for nested type as only bool,int,float,string are supported in ArrayStatistics

We should work on it as #45474 not this.

2- several statistics attributed has not implemented yet such as "ARROW:row_count:approximate","ARROW:average_byte_width:exact","ARROW:average_byte_width:approximate"

Could you open a new issue for this? Let's work it as a separated task.
(BTW, arrow::RecordBatch::MakeStatisticsArray() doesn't need to use ARROW:row_count:approximate because it always knows exact row count.)

[arashandishgar]

2- several statistics attributed has not implemented yet such as "ARROW:row_count:approximate","ARROW:average_byte_width:exact","ARROW:average_byte_width:approximate"

Could you open a new issue for this? Let's work it as a separated task. (BTW, arrow::RecordBatch::MakeStatisticsArray() doesn't need to use ARROW:row_count:approximate because it always knows exact row count.)

I've just created. #45639