Implement empty index for 0-rowed columns #1429

vasil-pashov · 2024-03-15T11:46:43Z

Reference Issues/PRs

Closes: #1428

What does this implement or fix?

Create an empty-index type. This required change in the Python and in the C++ layer.

In the C++ layer an new index type was added. (IndexDescriptor::EMPTY). It does not allocate a filed in the storage (similar to how row range index does not allocate a field). The checks for index compatibility are relaxed, the empty index is compatible with all other index types and it gets overridden the first time a non-empty index is written (either through update or append). On write we check if the dataframe contains 0 rows and if so it gets assigned an empty index.
The logic in the python layer is dodgy and needs discussion. In the current state the normalization metadata and the index descriptor are stored separately. There is one proto message describing both DateTime index and Ranged Index. The current change made it so that in case of 0 rows the python layer passes RowRange index to the C++ layer which checks if there are any rows in the DF. If there are rows Row range index is used, otherwise empty index is used. Note the is_not_range_index proto field. IMO it needs some refactoring in further PRs. It's used in the python layer to check if the first column is index or not.

Any other comments?

Merge this after: #1436

Checklist

Checklist for code changes...

Have you updated the relevant docstrings, documentation and copyright notice?
Is this contribution tested against all ArcticDB's features?
Do all exceptions introduced raise appropriate error messages?
Are API changes highlighted in the PR description?
Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

…n. Arctic no longer persists the dtype because empty columns are set to be of empty type. Which in Python terms is object dtype.

…ndex until empty index is created.

… tests for the compat-36 and compat-38 versions. The behavior shall be fixed with a later commit

…type

…allel

vasil-pashov · 2024-03-15T11:47:46Z

cpp/arcticdb/entity/types_proto.cpp

Moved from types_proto.hpp

vasil-pashov · 2024-03-15T11:48:05Z

cpp/arcticdb/entity/types_proto.hpp

Moved to types_proto.cpp

cpp/arcticdb/stream/index.hpp

cpp/arcticdb/python/python_to_tensor_frame.cpp

cpp/proto/arcticc/pb2/descriptors.proto

python/arcticdb/version_store/_normalization.py

cpp/arcticdb/python/python_to_tensor_frame.cpp

cpp/arcticdb/version/schema_checks.hpp

alexowens90 · 2024-03-19T11:08:13Z

cpp/arcticdb/version/schema_checks.hpp

    }
 }

-inline bool columns_match(const StreamDescriptor &left, const StreamDescriptor &right) {
-    if (left.fields().size() != right.fields().size())
+inline bool columns_match(const StreamDescriptor& left, const StreamDescriptor& right) {


It seems like this method should be symmetrical, but it does not handle the left non-empty/right empty case correctly

The right is not supposed to have empty index since modifying operations, except write, with 0 rows don't reach the C++ layer. Should I make it symmetric?

It's fine for it to not be symmetric, I would just rename the parameters to make it more obvious they can't be swapped

python/tests/unit/arcticdb/version_store/test_empty_column_type.py

Modify the normalization so that 0-rowed dataframe uniformly return Index([]) with dtype='object'

…rcticDB into proto_rename

alexowens90 · 2024-04-02T10:40:02Z

cpp/arcticdb/version/schema_checks.hpp

    }
 }

-inline bool columns_match(const StreamDescriptor &left, const StreamDescriptor &right) {
-    if (left.fields().size() != right.fields().size())
+inline bool columns_match(const StreamDescriptor& left, const StreamDescriptor& right) {


It's fine for it to not be symmetric, I would just rename the parameters to make it more obvious they can't be swapped

#### Reference Issues/PRs Relates to: #1429, #1440 #### What does this implement or fix? Feature flag the use of empty type. It uses the same library config option as the empty column type. In order to preserve behavior empty dataframes will use DataTimeIndex. #### Any other comments? #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>  --------- Co-authored-by: Vasil Pashov <[email protected]>

#### Reference Issues/PRs Closes: #1428 #### What does this implement or fix? Create an empty-index type. This required change in the Python and in the C++ layer. * In the C++ layer an new index type was added. (IndexDescriptor::EMPTY). It does not allocate a filed in the storage (similar to how row range index does not allocate a field). The checks for index compatibility are relaxed, the empty index is compatible with all other index types and it gets overridden the first time a non-empty index is written (either through update or append). On write we check if the dataframe contains 0 rows and if so it gets assigned an empty index. * The logic in the python layer is dodgy and needs discussion. In the current state the normalization metadata and the index descriptor are stored separately. There is one proto message describing both DateTime index and Ranged Index. The current change made it so that in case of 0 rows the python layer passes RowRange index to the C++ layer which checks if there are any rows in the DF. If there are rows Row range index is used, otherwise empty index is used. Note the `is_not_range_index` proto field. IMO it needs some refactoring in further PRs. It's used in the python layer to check if the first column is index or not. #### Any other comments? Merge this after: #1436 #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>  --------- Co-authored-by: Vasil Pashov <[email protected]>

#### Reference Issues/PRs Relates to: #1429, #1440 #### What does this implement or fix? Feature flag the use of empty type. It uses the same library config option as the empty column type. In order to preserve behavior empty dataframes will use DataTimeIndex. #### Any other comments? #### Checklist <details> <summary> Checklist for code changes... </summary> - [ ] Have you updated the relevant docstrings, documentation and copyright notice? - [ ] Is this contribution tested against [all ArcticDB's features](../docs/mkdocs/docs/technical/contributing.md)? - [ ] Do all exceptions introduced raise appropriate [error messages](https://docs.arcticdb.io/error_messages/)? - [ ] Are API changes highlighted in the PR description? - [ ] Is the PR labelled as enhancement or bug so it appears in autogenerated release notes? </details>  --------- Co-authored-by: Vasil Pashov <[email protected]>

Vasil Pashov and others added 15 commits February 27, 2024 18:21

New test cases

c81df09

Make default column type empty for 0 rowed columns

4b05988

Add additional empty-type tests

28aa366

Cosmetic changes to the empty-type tests

1450ccf

Remove a test testing that arctic preserves the type of 0-rowed colum…

7365c6c

…n. Arctic no longer persists the dtype because empty columns are set to be of empty type. Which in Python terms is object dtype.

Preserve the default behavour so that empty dateframes get datetime i…

dac194c

…ndex until empty index is created.

Remove non-reg tests because the desired behavior is different. Xfail…

47abdbd

… tests for the compat-36 and compat-38 versions. The behavior shall be fixed with a later commit

Xfail tests for older python/pandas versions

e0886a6

Merge branch 'master' into dev/vasil.pashov/fix-empty-column-default-…

b3762fa

…type

Make fixture deterministic so that the CI can distribute tests in par…

8494f3f

…allel

Create an empty index class and assign it to 0-rowed DFs

b667c71

Make empty index compatible with other index types

408d9be

Make it possible to append to and update dfs with empty index type

be8ab30

Fix comments and reduce test count

ec8b050

Merge branch 'master' into empty-index-type

4fa0659

vasil-pashov requested review from alexowens90 and willdealtry March 15, 2024 11:46

vasil-pashov commented Mar 15, 2024

View reviewed changes

cpp/arcticdb/entity/types_proto.cpp Outdated

Copy link

Collaborator Author

vasil-pashov Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved from types_proto.hpp

vasil-pashov commented Mar 15, 2024

View reviewed changes

cpp/arcticdb/entity/types_proto.hpp Outdated

Copy link

Collaborator Author

vasil-pashov Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to types_proto.cpp

Merge branch 'master' into empty-index-type

9601f70

willdealtry reviewed Mar 15, 2024

View reviewed changes

cpp/arcticdb/stream/index.hpp Show resolved Hide resolved

cpp/arcticdb/python/python_to_tensor_frame.cpp Outdated Show resolved Hide resolved

Vasil Pashov added 3 commits March 18, 2024 18:06

Roll back vcpkg version to fix failing abseil build

733934a

Merge branch 'master' into empty-index-type

fd30bda

Fix erros in tests

a2ce829

alexowens90 requested changes Mar 19, 2024

View reviewed changes

Vasil Pashov added 2 commits March 21, 2024 19:25

Rename is_not_ranged_index proto field in python and C++

7b0895f

Modify the normalization so that 0-rowed dataframe uniformly return Index([]) with dtype='object'

Fix failing tests

81871d9

vasil-pashov force-pushed the dev/vasil.pashov/empty-index branch from 22e63be to 81871d9 Compare March 28, 2024 16:59

Vasil Pashov and others added 2 commits March 29, 2024 15:55

Resolve review comments

f86640d

Merge branch 'master' into dev/vasil.pashov/empty-index

4e5a0eb

Vasil Pashov added 2 commits April 1, 2024 16:34

Handle multiindex

35ca8bc

Merge branch 'dev/vasil.pashov/empty-index' of github.com:man-group/A…

42db980

…rcticDB into proto_rename

alexowens90 requested changes Apr 2, 2024

View reviewed changes

Address review comment

292fe48

vasil-pashov changed the base branch from master to feature/empty_index April 2, 2024 11:53

alexowens90 approved these changes Apr 2, 2024

View reviewed changes

vasil-pashov merged commit a14eb48 into feature/empty_index Apr 2, 2024
108 of 109 checks passed

vasil-pashov deleted the dev/vasil.pashov/empty-index branch April 2, 2024 11:55

vasil-pashov mentioned this pull request Apr 5, 2024

Feature flag empty_index #1475

Merged

5 tasks

vasil-pashov mentioned this pull request Apr 18, 2024

Dynamic schema can append rowrange to datetime index leading to datacorruption #1507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement empty index for 0-rowed columns #1429

Implement empty index for 0-rowed columns #1429

vasil-pashov commented Mar 15, 2024 •

edited

Loading

vasil-pashov Mar 15, 2024

vasil-pashov Mar 15, 2024

alexowens90 Mar 19, 2024

vasil-pashov Mar 29, 2024

alexowens90 Apr 2, 2024

alexowens90 Apr 2, 2024

Implement empty index for 0-rowed columns #1429

Implement empty index for 0-rowed columns #1429

Conversation

vasil-pashov commented Mar 15, 2024 • edited Loading

Reference Issues/PRs

What does this implement or fix?

Any other comments?

Checklist

vasil-pashov Mar 15, 2024

Choose a reason for hiding this comment

vasil-pashov Mar 15, 2024

Choose a reason for hiding this comment

alexowens90 Mar 19, 2024

Choose a reason for hiding this comment

vasil-pashov Mar 29, 2024

Choose a reason for hiding this comment

alexowens90 Apr 2, 2024

Choose a reason for hiding this comment

alexowens90 Apr 2, 2024

Choose a reason for hiding this comment

vasil-pashov commented Mar 15, 2024 •

edited

Loading