Consolidate all sites that can hash a value inside a vector #9963

bikramSingh91 · 2024-05-29T01:26:37Z

Summary:
This commit aims to guarantee consistent evaluation and hashing of NaN
(Not-a-Number) values for floating-point types across multiple sites,
including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde.
Unit tests will be added incrementally to make it easier to review and to
include additional changes that might be required for it.

The changes to these classes would affect the following:

Multiple aggregates that have 'distinct' like semantics. Only for
the codepaths that consume complex types as inputs. Aggregates
include: multimap_agg, map_union_sum, map_union, count(distinct)
In-predicate
Map_subscript, only one optimization code path for
constant/dictionary encoded map
Hash Partitioning operator (HashPartition)
Hash Join operator (HashProbe, HashTable)
Local partitioning operator (LocalPartition)
Hash Aggregation operator (GroupingSet)

Differential Revision: D57892204

facebook-github-bot · 2024-05-29T01:26:45Z

This pull request was exported from Phabricator. Differential Revision: D57892204

netlify · 2024-05-29T01:26:54Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`7470b81`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/665a18d3f211ff0008941ec1

…incubator#9963) Summary: This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Differential Revision: D57892204

facebook-github-bot · 2024-05-29T17:48:57Z

This pull request was exported from Phabricator. Differential Revision: D57892204

…incubator#9963) Summary: This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Differential Revision: D57892204

facebook-github-bot · 2024-05-29T17:52:07Z

This pull request was exported from Phabricator. Differential Revision: D57892204

kagamiori

LGTM. Since many functions or operators are affected and you plan to add unit tests for them incrementally, do you plan to land this PR first or after those unit tests are added?

kagamiori · 2024-05-31T16:36:14Z

velox/exec/tests/VectorHasherTest.cpp

+  vectorHasher->decode(*vector, allRows);
+  vectorHasher->hash(allRows, false, hashes);
+  std::vector<uint64_t> expected = {
+      hasher(1.0), hasher(-1.0), kNaNHash, kNaNHash, hasher(0.0)};


Could we also test -0.0 here?

…incubator#9963) Summary: This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Reviewed By: kagamiori Differential Revision: D57892204

facebook-github-bot · 2024-05-31T18:36:01Z

This pull request was exported from Phabricator. Differential Revision: D57892204

…incubator#9963) Summary: This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Reviewed By: kagamiori Differential Revision: D57892204

facebook-github-bot · 2024-05-31T18:37:23Z

This pull request was exported from Phabricator. Differential Revision: D57892204

facebook-github-bot · 2024-05-31T23:10:56Z

This pull request has been merged in e99972b.

conbench-facebook · 2024-05-31T23:53:19Z

Conbench analyzed the 1 benchmark run on commit e99972bb.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Ensure NaNs values of any binary representations are treated as equal and can be identified as keys in a map. Summary of changes: - For primitive type keys: Use a map that employs the correct hashing and equality operators for floating types. - For complex type keys: It uses GenericView as a key that uses BaseVector's hashing and equality operators. Those recursively call SimpleVector's respective functions that were addressed in facebookincubator#9963 Differential Revision: D57681657

Summary: Pull Request resolved: #9893 Ensure NaNs values of any binary representations are treated as equal and can be identified as keys in a map. Summary of changes: - For primitive type keys: Use a map that employs the correct hashing and equality operators for floating types. - For complex type keys: It uses GenericView as a key that uses BaseVector's hashing and equality operators. Those recursively call SimpleVector's respective functions that were addressed in #9963 Reviewed By: spershin Differential Revision: D57681657 fbshipit-source-id: 0ce281f303dd0633e698e4b386627c3831567d92

Summary: Ensures that NaNs with different binary representations are considered equal and deduplicated. Summary of changes: For primitive types, we ensure that the Set data structure used to deduplicate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in facebookincubator#9963 Differential Revision: D58161333

Summary: Ensures that NaNs with different binary representations are considered equal and deduplicated when used as keys in a map. Summary of changes: For primitive types, we ensure that the Map data structure used to accumulate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in facebookincubator#9963 Differential Revision: D58209683

Summary: The map data structure used in multimap_agg that handles complex keys uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as its hash and equals functions respectively. These were fixed in facebookincubator#9963 to handle NaN values correctly. Therefore, this change adds a unit to verify handling of complex keys that can contain NaN values. Differential Revision: D58212803

…ebookincubator#10074) Summary: The map data structure used in multimap_agg that handles complex keys uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as its hash and equals functions respectively. These were fixed in facebookincubator#9963 to handle NaN values correctly. Therefore, this change adds a unit to verify handling of complex keys that can contain NaN values. Differential Revision: D58212803

…ncubator#10047) Summary: Ensures that NaNs with different binary representations are considered equal and deduplicated. Summary of changes: All these aggs use SetAccumulator where: For primitive types, we ensure that the Set data structure used to deduplicate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in facebookincubator#9963 Differential Revision: D58161333

…incubator#9963) Summary: Pull Request resolved: facebookincubator#9963 This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Reviewed By: kagamiori Differential Revision: D57892204 fbshipit-source-id: 3729aec02c597379c8fbb1d2eec190614aa57d43

Summary: Pull Request resolved: facebookincubator#9893 Ensure NaNs values of any binary representations are treated as equal and can be identified as keys in a map. Summary of changes: - For primitive type keys: Use a map that employs the correct hashing and equality operators for floating types. - For complex type keys: It uses GenericView as a key that uses BaseVector's hashing and equality operators. Those recursively call SimpleVector's respective functions that were addressed in facebookincubator#9963 Reviewed By: spershin Differential Revision: D57681657 fbshipit-source-id: 0ce281f303dd0633e698e4b386627c3831567d92

…incubator#9963) Summary: Pull Request resolved: facebookincubator#9963 This commit aims to guarantee consistent evaluation and hashing of NaN (Not-a-Number) values for floating-point types across multiple sites, including SimpleVector, VectorHasher, RowContainer, and ContainerRowSerde. Unit tests will be added incrementally to make it easier to review and to include additional changes that might be required for it. The changes to these classes would affect the following: - Multiple aggregates that have 'distinct' like semantics. Only for the codepaths that consume complex types as inputs. Aggregates include: multimap_agg, map_union_sum, map_union, count(distinct) - In-predicate - Map_subscript, only one optimization code path for constant/dictionary encoded map - Hash Partitioning operator (HashPartition) - Hash Join operator (HashProbe, HashTable) - Local partitioning operator (LocalPartition) - Hash Aggregation operator (GroupingSet) Reviewed By: kagamiori Differential Revision: D57892204 fbshipit-source-id: 3729aec02c597379c8fbb1d2eec190614aa57d43

Summary: Pull Request resolved: facebookincubator#9893 Ensure NaNs values of any binary representations are treated as equal and can be identified as keys in a map. Summary of changes: - For primitive type keys: Use a map that employs the correct hashing and equality operators for floating types. - For complex type keys: It uses GenericView as a key that uses BaseVector's hashing and equality operators. Those recursively call SimpleVector's respective functions that were addressed in facebookincubator#9963 Reviewed By: spershin Differential Revision: D57681657 fbshipit-source-id: 0ce281f303dd0633e698e4b386627c3831567d92

Summary: Ensure NaN values of different binary representation for floating point types are considered as equal. Summary of changes: - Primitive type Input: NaN of different binary representations are denormalized to the same representation before adding to the in-list and before being compared. - Complex Type input: Uses a set that employs hash and equality functions via BaseVector that have been fixed in facebookincubator#9963 to handle NaN values. Differential Revision: D58301120

Summary: Pull Request resolved: facebookincubator#10115 Ensure NaN values of different binary representation for floating point types are considered as equal. Summary of changes: - Primitive type Input: NaN of different binary representations are denormalized to the same representation before adding to the in-list and before being compared. - Complex Type input: Uses a set that employs hash and equality functions via BaseVector that have been fixed in facebookincubator#9963 to handle NaN values. Differential Revision: D58301120

Summary: Hash join and hash aggregation employ data structures like VectorHasher, RowContainer and ContainerRowSerde apart from the usual BaseVector to implement a custom hash table and store intermediate state. All these have been updated in facebookincubator#9963 to handle NaN values where all binary representations of the same are considered equal and hash to the same value. Therefore, this change only adds unit tests to verify the expected NaN behavior. Differential Revision: D58365384

Summary: Pull Request resolved: facebookincubator#9893 Ensure NaNs values of any binary representations are treated as equal and can be identified as keys in a map. Summary of changes: - For primitive type keys: Use a map that employs the correct hashing and equality operators for floating types. - For complex type keys: It uses GenericView as a key that uses BaseVector's hashing and equality operators. Those recursively call SimpleVector's respective functions that were addressed in facebookincubator#9963 Reviewed By: spershin Differential Revision: D57681657 fbshipit-source-id: 0ce281f303dd0633e698e4b386627c3831567d92

…ncubator#10125) Summary: Pull Request resolved: facebookincubator#10125 Hash join and hash aggregation employ data structures like VectorHasher, RowContainer and ContainerRowSerde apart from the usual BaseVector to implement a custom hash table and store intermediate state. All these have been updated in facebookincubator#9963 to handle NaN values where all binary representations of the same are considered equal and hash to the same value. Therefore, this change only adds unit tests to verify the expected NaN behavior. Differential Revision: D58365384

Summary: Pull Request resolved: #10125 Hash join and hash aggregation employ data structures like VectorHasher, RowContainer and ContainerRowSerde apart from the usual BaseVector to implement a custom hash table and store intermediate state. All these have been updated in #9963 to handle NaN values where all binary representations of the same are considered equal and hash to the same value. Therefore, this change only adds unit tests to verify the expected NaN behavior. Reviewed By: kagamiori Differential Revision: D58365384 fbshipit-source-id: 11d7d6d760295b12b42cd24a7c2fcf567a26bd24

) Summary: Pull Request resolved: #10074 The map data structure used in multimap_agg that handles complex keys uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as its hash and equals functions respectively. These were fixed in #9963 to handle NaN values correctly. Therefore, this change adds a unit to verify handling of complex keys that can contain NaN values. Reviewed By: kagamiori Differential Revision: D58212803 fbshipit-source-id: 81cbaa823baea1f4d42bbc32bb7ca5602634506c

) Summary: Pull Request resolved: #10071 Ensures that NaNs with different binary representations are considered equal and deduplicated when used as keys in a map. Summary of changes: For primitive types, we ensure that the Map data structure used to accumulate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in #9963 Reviewed By: kgpai Differential Revision: D58209683 fbshipit-source-id: f8697f248a0346456597ca71105eb5bcf1b71f8b

…ncubator#10047) Summary: Pull Request resolved: facebookincubator#10047 Ensures that NaNs with different binary representations are considered equal and deduplicated. Summary of changes: All these aggs use SetAccumulator where: For primitive types, we ensure that the Set data structure used to deduplicate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in facebookincubator#9963 Differential Revision: D58161333

…ncubator#10047) Summary: Pull Request resolved: facebookincubator#10047 Ensures that NaNs with different binary representations are considered equal and deduplicated. Summary of changes: All these aggs use SetAccumulator where: For primitive types, we ensure that the Set data structure used to deduplicate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in facebookincubator#9963 Reviewed By: kgpai Differential Revision: D58161333

Summary: Pull Request resolved: #10047 Ensures that NaNs with different binary representations are considered equal and deduplicated. Summary of changes: All these aggs use SetAccumulator where: For primitive types, we ensure that the Set data structure used to deduplicate uses hash and equality functions that handle NaN. For complex input it uses BaseVector::hashValueAt() and ContainerRowSerde::compare() as a hash and equality function, respectively which were fixed to handle NaNs properly in #9963 Reviewed By: kgpai Differential Revision: D58161333 fbshipit-source-id: d7db9ec2e86d0f87bc0a60b6e238ba95135d7eb1

Summary: Pull Request resolved: facebookincubator#10115 Ensure NaN values of different binary representation for floating point types are considered as equal. Summary of changes: - Primitive type Input: NaN of different binary representations are denormalized to the same representation before adding to the in-list and before being compared. - Complex Type input: Uses a set that employs hash and equality functions via BaseVector that have been fixed in facebookincubator#9963 to handle NaN values. Reviewed By: kagamiori Differential Revision: D58301120

Summary: Pull Request resolved: #10115 Ensure NaN values of different binary representation for floating point types are considered as equal. Summary of changes: - Primitive type Input: NaN of different binary representations are denormalized to the same representation before adding to the in-list and before being compared. - Complex Type input: Uses a set that employs hash and equality functions via BaseVector that have been fixed in #9963 to handle NaN values. Reviewed By: kagamiori Differential Revision: D58301120 fbshipit-source-id: 80b6958a8d9062c80e67ba2159a9a9093b236849

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 29, 2024

facebook-github-bot added the fb-exported label May 29, 2024

bikramSingh91 mentioned this pull request May 29, 2024

Fix NaN handling in map_subset #9893

Closed

bikramSingh91 force-pushed the export-D57892204 branch from 40ae068 to 4c96c65 Compare May 29, 2024 17:48

bikramSingh91 force-pushed the export-D57892204 branch from 4c96c65 to 908fd36 Compare May 29, 2024 17:51

bikramSingh91 mentioned this pull request May 29, 2024

Fix NaN handling for map subscript and add test for map() #9881

Closed

kagamiori approved these changes May 31, 2024

View reviewed changes

bikramSingh91 force-pushed the export-D57892204 branch from 908fd36 to c242103 Compare May 31, 2024 18:35

bikramSingh91 force-pushed the export-D57892204 branch from c242103 to 7470b81 Compare May 31, 2024 18:37

facebook-github-bot closed this in e99972b May 31, 2024

facebook-github-bot added the Merged label May 31, 2024

bikramSingh91 mentioned this pull request Jun 4, 2024

Fix NaN handling in set_agg, set_union and count(distinct) #10047

Closed

bikramSingh91 mentioned this pull request Jun 5, 2024

Fix NaN handling for histogram, map_agg and map_union aggregates #10071

Closed

bikramSingh91 mentioned this pull request Jun 5, 2024

Add test for multimap_agg for complex keys containing NaN values #10074

Closed

bikramSingh91 mentioned this pull request Jun 7, 2024

Fix NaN handling for in-predicate #10115

Closed

zeodtr mentioned this pull request Jun 10, 2024

VectorHasher's value ID caching logic makes certain queries unnecessarily slow #10057

Closed

bikramSingh91 mentioned this pull request Jun 10, 2024

Add tests for join and aggregation related to NaN handling #10125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate all sites that can hash a value inside a vector #9963

Consolidate all sites that can hash a value inside a vector #9963

bikramSingh91 commented May 29, 2024

facebook-github-bot commented May 29, 2024

netlify bot commented May 29, 2024 •

edited

Loading

facebook-github-bot commented May 29, 2024

facebook-github-bot commented May 29, 2024

kagamiori left a comment •

edited

Loading

kagamiori May 31, 2024

facebook-github-bot commented May 31, 2024

facebook-github-bot commented May 31, 2024

facebook-github-bot commented May 31, 2024

conbench-facebook bot commented May 31, 2024

Consolidate all sites that can hash a value inside a vector #9963

Consolidate all sites that can hash a value inside a vector #9963

Conversation

bikramSingh91 commented May 29, 2024

facebook-github-bot commented May 29, 2024

netlify bot commented May 29, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

facebook-github-bot commented May 29, 2024

facebook-github-bot commented May 29, 2024

kagamiori left a comment • edited Loading

Choose a reason for hiding this comment

kagamiori May 31, 2024

Choose a reason for hiding this comment

facebook-github-bot commented May 31, 2024

facebook-github-bot commented May 31, 2024

facebook-github-bot commented May 31, 2024

conbench-facebook bot commented May 31, 2024

netlify bot commented May 29, 2024 •

edited

Loading

kagamiori left a comment •

edited

Loading