feat: Allow `row_range` to be treated as a clause #864

jjerphan · 2023-09-15T07:29:47Z

Reference Issues/PRs

Fixes #747.

What does this implement/fix? How does it work (high level)? Highlight notable design decisions.

Any other comments?

Checklist

Checklist for code changes...

Have you updated the relevant docstrings and documentation?
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?

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

This is equivalent but this has the benefit of allowing efining defaults value in Python 3.6, which aren't available via the `namedtuple` factory (only have been added in Python 3.7). See: https://docs.python.org/3/library/typing.html#typing.NamedTuple See: https://docs.python.org/3/library/collections.html#collections.namedtuple Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

This is suboptimal, but for now slicing on columns makes the composition with other clauses (like FilterClause) impossible. Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

cpp/arcticdb/processing/clause.cpp

jjerphan · 2023-10-05T08:21:46Z

python/arcticdb/version_store/processing.py

+class PythonRowRangeClause(NamedTuple):
+ row_range_type: _RowRangeType = None
+ n: int = None
+ start: int = None
+ end: int = None


Note to reviewers: using the NamedTuple class allows specifying default values to use either:

PythonRowRangeClause(row_range_type=_RowRangeType.HEAD, n=n))

PythonRowRangeClause(row_range_type=_RowRangeType.TAIL, n=n))

PythonRowRangeClause(start=start, end=end))

jjerphan · 2023-10-05T08:34:51Z

python/arcticdb/version_store/_store.py

@@ -1498,7 +1520,8 @@ def _get_read_query(self, date_range: Optional[DateRangeInput], row_range, colum
 read_query.row_filter = _normalize_dt_range(date_range)

 if row_range is not None:
- read_query.row_range = row_range
+ total_n_rows = self.get_num_rows(symbol=symbol, as_of=as_of, **kwargs)


Note to reviewers: get_description is an alternative, but this is unreachable in this scope.

The reason the logic in _normalize_row_range was in the C++ layer was because methods like get_num_rows involve reading the index key, so it will be read twice with this implementation. Can we please refactor back to the state where it is only read once?

The reason I did this change was to get ride of calculate_row_filter and SignedRowRange in cpp/arcticdb/pipeline/query.hpp.

I first tried to have this logic in the C++ side but I do not know what is the best way to get the total number of lines as soon as possible unfortunately.

it will be read twice with this implementation

How costly is this? Is there a way to get the total number of rows without this cost?

Reverted back with e2b97e9.

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

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

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

python/arcticdb/version_store/_store.py

python/arcticdb/version_store/processing.py

cpp/arcticdb/processing/clause.hpp

cpp/arcticdb/processing/clause.cpp

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Alex Owens <alex.owens@man.com>

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan force-pushed the feat/read-row_range-clause branch 2 times, most recently from 40aedec to b83701b Compare September 15, 2023 08:15

feat: Allow row_range to be treated as a clause

45ca483

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan force-pushed the feat/read-row_range-clause branch from b83701b to 45ca483 Compare September 15, 2023 08:31

jjerphan added 2 commits September 15, 2023 11:52

Use signed_row_range for consistency

ff2bce3

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Add row_range to ReadRequest

2fdea1c

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan force-pushed the feat/read-row_range-clause branch from 639b4a5 to a811ae1 Compare September 15, 2023 12:06

jjerphan added 2 commits September 15, 2023 14:44

Extend batch methods for row_range

ed2dfe0

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan force-pushed the feat/read-row_range-clause branch from a811ae1 to 208ab56 Compare September 15, 2023 12:46

jjerphan added 11 commits September 15, 2023 15:01

Move SignedRowRange down the python front-end

c102d5e

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Allow to chain on QueryBuilder._row_range

df415aa

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

test: Add tests for row_range clause

942cdf2

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Removed SignedRowRange entirely

885136e

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

python: Row Range normalization

86f2a23

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

test: Add a simple test for the RowRange clause

812c68a

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Make RowRangeClause slices on rows instead

2b996f8

This is suboptimal, but for now slicing on columns makes the composition with other clauses (like FilterClause) impossible. Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

test: Fix test

e36903a

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Merge branch 'master' into feat/read-row_range-clause

1ca6ba3

Wrap around negative indices

53a012e

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

Rework test regarding range exclusivity

a33bc6b

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan commented Oct 5, 2023

View reviewed changes

jjerphan marked this pull request as ready for review October 5, 2023 08:35

Revert changes to the C++ layer

e2b97e9

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan requested a review from alexowens90 October 17, 2023 12:08

test: Remove test_normalize_row_range

68e0fa7

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

alexowens90 requested changes Oct 19, 2023

View reviewed changes

jjerphan and others added 2 commits October 19, 2023 16:46

Address review comments

74c7ea3

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Alex Owens <alex.owens@man.com>

fixup! Address review comments

02c026b

docs: Reword comment

27a4a44

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

alexowens90 approved these changes Oct 24, 2023

View reviewed changes

wip: Edge case for empty ChunkedBufferImpl

8e4a390

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan force-pushed the feat/read-row_range-clause branch from ee78af7 to 8e4a390 Compare October 25, 2023 15:12

jjerphan added 3 commits October 31, 2023 11:22

Merge branch 'master' into feat/read-row_range-clause

24f8e36

wip: Resolve segmentation fault on Segment truncation

8e16ae8

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

fix: Resolve segmentation fault on Segment truncation

50bebc0

Signed-off-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan added the enhancement New feature or request label Nov 3, 2023

jjerphan merged commit bd078ff into master Nov 3, 2023
98 checks passed

jjerphan deleted the feat/read-row_range-clause branch November 3, 2023 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow `row_range` to be treated as a clause #864

feat: Allow `row_range` to be treated as a clause #864

jjerphan commented Sep 15, 2023

jjerphan Oct 5, 2023

jjerphan Oct 5, 2023

alexowens90 Oct 9, 2023

jjerphan Oct 10, 2023

jjerphan Oct 17, 2023 •

edited

Loading

jjerphan Oct 17, 2023

feat: Allow row_range to be treated as a clause #864

feat: Allow row_range to be treated as a clause #864

Conversation

jjerphan commented Sep 15, 2023

Reference Issues/PRs

What does this implement/fix? How does it work (high level)? Highlight notable design decisions.

Any other comments?

Checklist

jjerphan Oct 5, 2023

Choose a reason for hiding this comment

jjerphan Oct 5, 2023

Choose a reason for hiding this comment

alexowens90 Oct 9, 2023

Choose a reason for hiding this comment

jjerphan Oct 10, 2023

Choose a reason for hiding this comment

jjerphan Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

jjerphan Oct 17, 2023

Choose a reason for hiding this comment

feat: Allow `row_range` to be treated as a clause #864

feat: Allow `row_range` to be treated as a clause #864

jjerphan Oct 17, 2023 •

edited

Loading