GH-28074: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[sanjibansg]

This PR fixes the issue of handling NaNs in the Parquet predicate push-down.
While computing the valid bounds for a column, if the max or min of the column is null, the range should ignore that.

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12264

[westonpace]

This looks like the right logic but I think we should be checking for NaN and not using is_valid (which checks for null).

So something like...

bool isNan(const Scalar& scalar) {
if (IsFloat(scalar)) {
const FloatScalar& float_scalar = checked_cast<const FloatScalar&>(scalar);
return isnan(float_scalar);
} else if (IsDouble(scalar)) {
// ...
} else {
return false;
}
}

[westonpace]

This looks correct to me. @pitrou did you want to do a quick sanity check?

How hard would it be to mock up some kind of test to check this? Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

[pitrou]

Unfortunately, ColumnChunkStatisticsAsExpression isn't really a public method and so it might be tricky coming up with a test case without reproducing an offending parquet file which may not be easy to do.

I agree we should ideally unit test ColumnChunkStatisticsAsExpression. Perhaps it can be exposed as an internal API?

[github-actions]

• Closes: [C++][Dataset] Handle NaNs correctly in Parquet predicate push-down #28074

[github-actions]

⚠️ GitHub issue #28074has been automatically assigned in GitHub to PR creator.

[wjones127]

I believe I have generated a file that could be used for a test case here: apache/parquet-testing#35

Does that seem sufficient?

[westonpace]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

[sanjibansg]

Looks like the parquet tests are failing because they can't find the parquet file. Do you maybe need to update the submodule version? I haven't done this recently and am not sure of the right commands.

@westonpace
I think by mistake, I updated the arrow-testing submodule as well while updating parquet-testing. Will it be okay?

[westonpace]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

[sanjibansg]

This looks good. I don't see a problem with updating the arrow testing repo but it might be nice to keep changes isolated if you can.

Yes, of course, and sorry, I will be more careful next time.

[westonpace]

One minor nit

[westonpace]

Thanks!

[ursabot]

Benchmark runs are scheduled for baseline = 0a7e7fb and contender = 518fc51. 518fc51 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed] test-mac-arm
[Failed] ursa-i9-9960x
[Finished ⬇️0.1% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 518fc51e ec2-t3-xlarge-us-east-2
[Failed] 518fc51e test-mac-arm
[Finished] 518fc51e ursa-i9-9960x
[Finished] 518fc51e ursa-thinkcentre-m75q
[Failed] 0a7e7fb1 ec2-t3-xlarge-us-east-2
[Failed] 0a7e7fb1 test-mac-arm
[Failed] 0a7e7fb1 ursa-i9-9960x
[Finished] 0a7e7fb1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java