perf: Optimize column_encryption_policy checks in recv_results_rows (1.7x speedup for Cython, 1.2x for Python) - #630
perf: Optimize column_encryption_policy checks in recv_results_rows (1.7x speedup for Cython, 1.2x for Python)#630mykaul wants to merge 5 commits into
Conversation
|
CI failure is unrelated, and I've seen it happening on too many PRs already :-/ |
3a9031e to
ad8b6f9
Compare
dkropachev
left a comment
dkropachev
left a comment
There was a problem hiding this comment.
numpy_parser.pyx, parsing.pyx also needs to be fixed
76c09ae to
f44a9ba
Compare
It wasn't clear to me what needs to fixing there: |
Instead of changing only While Abstract classes: |
Let me see if I understand this:
Is that an accurate representation?
I'm not sure I understand the interaction between Python and Cython - I was not going to change the interfaces whatsoever.
You raise a good point about the names of the functions - that can clearly be more clear with 'plain' and 'encrypted' in them! I'll fix that. |
f44a9ba to
132859f
Compare
132859f to
d87f917
Compare
Done in latest commit. I think it's ready for re-review. |
|
CI failure seems like known issue #446 |
|
Can this be reviewed? In my (limited) testing, it did show some nice improvement. It's just a branch or so, but apparently, was impacting to some degree the performance. |
…nt.bind() When Cython serializers (from cassandra.serializers) are available and no column encryption policy is active, BoundStatement.bind() now uses pre-built Serializer objects cached on the PreparedStatement instead of calling cqltype classmethods. This avoids per-value Python method dispatch overhead and enables the ~30x vector serialization speedup from the Cython serializers module. The bind loop is split into three paths: 1. Column encryption policy path (unchanged behavior) 2. Cython serializers path (new fast path) 3. Plain Python path (no CE, no Cython -- removes per-value ColDesc/CE check) Depends on PR scylladb#748 (Cython serializers module) and PR scylladb#630 (CE-policy bind split).
…nt.bind() When Cython serializers (from cassandra.serializers) are available and no column encryption policy is active, BoundStatement.bind() now uses pre-built Serializer objects cached on the PreparedStatement instead of calling cqltype classmethods. This avoids per-value Python method dispatch overhead and enables the ~30x vector serialization speedup from the Cython serializers module. The bind loop is split into three paths: 1. Column encryption policy path (unchanged behavior) 2. Cython serializers path (new fast path) 3. Plain Python path (no CE, no Cython -- removes per-value ColDesc/CE check) Depends on PR scylladb#748 (Cython serializers module) and PR scylladb#630 (CE-policy bind split).
…nt.bind() When Cython serializers (from cassandra.serializers) are available and no column encryption policy is active, BoundStatement.bind() now uses pre-built Serializer objects cached on the PreparedStatement instead of calling cqltype classmethods. This avoids per-value Python method dispatch overhead and enables the ~30x vector serialization speedup from the Cython serializers module. The bind loop is split into three paths: 1. Column encryption policy path (unchanged behavior) 2. Cython serializers path (new fast path) 3. Plain Python path (no CE, no Cython -- removes per-value ColDesc/CE check) Depends on PR scylladb#748 (Cython serializers module) and PR scylladb#630 (CE-policy bind split).
Split recv_results_rows into fast path (no column encryption) and slow
path (column encryption enabled):
Fast path (common case):
- Reads raw column bytes and decodes types in a single pass per row
via _decode_row_inline(), eliminating the intermediate list-of-lists
- Skips ColDesc namedtuple creation entirely (only needed for CE)
- No closure allocation per call
Slow path (column encryption):
- Preserves full CE logic with ColDesc creation
- Moves decode_val/decode_row closures to module-level functions
(_decode_val_ce, _decode_row_ce) to avoid per-call closure overhead
Note: This PR modifies the same method as PR scylladb#630 (which also splits
recv_results_rows into CE/non-CE branches). There will be a merge
conflict that needs manual resolution if both PRs are accepted.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change separates encrypted and plain value serialization and result decoding. Cython row parsers now expose dedicated encrypted and plain unpacking methods. NumPy parsing rejects encrypted columns. Row descriptor construction and exception handling select the appropriate path. Tests cover policy-aware decoding, Cython parsing, call-count optimization, descriptor validation, and NumPy behavior. Sequence Diagram(s)sequenceDiagram
participant BoundStatement
participant EncryptionPolicy
participant ResultMessage
participant CythonParser
BoundStatement->>EncryptionPolicy: determine encrypted columns
BoundStatement->>EncryptionPolicy: serialize and encrypt selected values
ResultMessage->>EncryptionPolicy: precompute column encryption decisions
ResultMessage->>EncryptionPolicy: decrypt encrypted values
ResultMessage->>CythonParser: select encrypted or plain row parser
CythonParser-->>ResultMessage: decoded rows
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
Rebased onto current No other conflicts — Ran One note for awareness: #765 (draft, also mine) independently optimizes the pure-Python |
There was a problem hiding this comment.
Pull request overview
Optimizes result parsing and prepared-statement binding by selecting encrypted or plain paths once per operation.
Changes:
- Splits Python and Cython row decoding into encrypted/plain paths.
- Adds NumPy encryption rejection and parser tests.
- Optimizes
BoundStatement.bind()policy checks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_protocol.py |
Adds decoding and parser tests. |
cassandra/row_parser.pyx |
Selects the row parser path once. |
cassandra/query.py |
Splits encrypted/plain binding paths. |
cassandra/protocol.py |
Splits pure-Python result decoding. |
cassandra/parsing.pyx |
Defines separate row parser methods. |
cassandra/parsing.pxd |
Updates Cython parser declarations. |
cassandra/obj_parser.pyx |
Implements optimized tuple decoding paths. |
cassandra/numpy_parser.pyx |
Rejects column encryption configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cassandra/numpy_parser.pyx:86
- This exception is swallowed in production by
make_recv_results_rows, whose broadexcept Exceptionfallback rewinds the buffer and successfully parses encrypted rows withTupleRowParserinstead. ConsequentlyNumpyProtocolHandlerreturns withparsed_rowsstill unset rather than rejecting the unsupported configuration; the test only callsNumpyParserdirectly and misses the wrapper behavior.
if desc.column_encryption_policy:
raise NotImplementedError(
"NumpyParser does not support column encryption")
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cassandra/row_parser.pyx (1)
40-53: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRe-raise
NotImplementedErrorbefore the fallback decode
colparser.parse_rows(reader, desc)is wrapped by a blanketexcept Exception, soNumpyParser.parse_rows()’sNotImplementedErrorfor encrypted results gets caught here. TheTupleRowParserfallback then decodes the rows successfully but never assignsself.parsed_rows, soNumpyProtocolHandlercan return with missing results instead of failing on unsupported column encryption.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/row_parser.pyx` around lines 40 - 53, Update the exception handling around colparser.parse_rows in the row parsing flow to immediately re-raise NotImplementedError before entering the TupleRowParser fallback. Preserve the existing fallback for other decoding exceptions, ensuring unsupported encrypted results fail explicitly instead of continuing without assigning self.parsed_rows.
🧹 Nitpick comments (2)
tests/unit/test_protocol.py (1)
799-814: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest bypasses the real
NumpyProtocolHandlercodepath.This test calls
NumpyParser().parse_rows(reader, desc)directly. In production,NumpyProtocolHandlerinvokes this throughmake_recv_results_rows'srecv_results_rowswrapper (cassandra/row_parser.pyx), whose broadexcept Exceptioncurrently swallows this exactNotImplementedErrorand silently falls back toTupleRowParserwithout settingself.parsed_rows(see comment on cassandra/row_parser.pyx). A test that exercisesrecv_results_rowsend-to-end (e.g., viamake_recv_results_rows(NumpyParser())) would have caught this.Want me to draft this additional test case?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_protocol.py` around lines 799 - 814, The test should exercise the production NumpyProtocolHandler path rather than calling NumpyParser.parse_rows directly. Update test_numpy_parser_rejects_encryption to invoke make_recv_results_rows(NumpyParser()) and its recv_results_rows wrapper with the same reader and parse description, then assert NotImplementedError propagates instead of being swallowed or falling back to TupleRowParser.cassandra/query.py (1)
687-731: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated boilerplate from splitting encrypted/plain paths. Both sites split a single per-value loop into two policy-specific loops (as intended by this PR), but in doing so duplicated the surrounding value-guard/error-handling boilerplate verbatim, rather than only branching on the actual serialize/deserialize step.
cassandra/query.py#L687-L731: extract theNone/UNSET_VALUEhandling and theexcept (TypeError, struct.error)message construction into a single loop, branching only on howcol_bytesis computed (a small_serialize(value, col_spec)closure per branch), instead of duplicating the whole loop body.cassandra/obj_parser.pyx#L69-L128: factor the identicalexcept Exception as e: raise DriverException(...)block inunpack_col_encrypted_row/unpack_plain_rowinto a shared helper (e.g. acdeffunction takingdesc,i,e), so future changes to the error message only need to be made once.♻️ Example for query.py
- if ce_policy: - for value, col_spec in zip(values, col_meta): - if value is None: - self.values.append(None) - elif value is UNSET_VALUE: - ... - else: - try: - col_desc = ColDesc(col_spec.keyspace_name, col_spec.table_name, col_spec.name) - uses_ce = ce_policy.contains_column(col_desc) - if uses_ce: - col_type = ce_policy.column_type(col_desc) - col_bytes = col_type.serialize(value, proto_version) - col_bytes = ce_policy.encrypt(col_desc, col_bytes) - else: - col_type = col_spec.type - col_bytes = col_type.serialize(value, proto_version) - self.values.append(col_bytes) - except (TypeError, struct.error) as exc: - ... - else: - for value, col_spec in zip(values, col_meta): - if value is None: - self.values.append(None) - elif value is UNSET_VALUE: - ... - else: - try: - col_type = col_spec.type - col_bytes = col_type.serialize(value, proto_version) - self.values.append(col_bytes) - except (TypeError, struct.error) as exc: - ... + if ce_policy: + def _serialize(value, col_spec): + col_desc = ColDesc(col_spec.keyspace_name, col_spec.table_name, col_spec.name) + if ce_policy.contains_column(col_desc): + col_bytes = ce_policy.column_type(col_desc).serialize(value, proto_version) + return ce_policy.encrypt(col_desc, col_bytes) + return col_spec.type.serialize(value, proto_version) + else: + def _serialize(value, col_spec): + return col_spec.type.serialize(value, proto_version) + + for value, col_spec in zip(values, col_meta): + if value is None: + self.values.append(None) + elif value is UNSET_VALUE: + ... + else: + try: + self.values.append(_serialize(value, col_spec)) + except (TypeError, struct.error) as exc: + ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cassandra/query.py` around lines 687 - 731, In cassandra/query.py lines 687-731, consolidate the encrypted and plain serialization paths into one value loop, reusing shared None/UNSET_VALUE guards and TypeError/struct.error handling while branching only through a small per-branch serializer for col_bytes. In cassandra/obj_parser.pyx lines 69-128, extract the identical exception-to-DriverException logic from unpack_col_encrypted_row and unpack_plain_row into a shared cdef helper accepting desc, i, and the exception, and use it from both functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cassandra/row_parser.pyx`:
- Around line 40-53: Update the exception handling around colparser.parse_rows
in the row parsing flow to immediately re-raise NotImplementedError before
entering the TupleRowParser fallback. Preserve the existing fallback for other
decoding exceptions, ensuring unsupported encrypted results fail explicitly
instead of continuing without assigning self.parsed_rows.
---
Nitpick comments:
In `@cassandra/query.py`:
- Around line 687-731: In cassandra/query.py lines 687-731, consolidate the
encrypted and plain serialization paths into one value loop, reusing shared
None/UNSET_VALUE guards and TypeError/struct.error handling while branching only
through a small per-branch serializer for col_bytes. In cassandra/obj_parser.pyx
lines 69-128, extract the identical exception-to-DriverException logic from
unpack_col_encrypted_row and unpack_plain_row into a shared cdef helper
accepting desc, i, and the exception, and use it from both functions.
In `@tests/unit/test_protocol.py`:
- Around line 799-814: The test should exercise the production
NumpyProtocolHandler path rather than calling NumpyParser.parse_rows directly.
Update test_numpy_parser_rejects_encryption to invoke
make_recv_results_rows(NumpyParser()) and its recv_results_rows wrapper with the
same reader and parse description, then assert NotImplementedError propagates
instead of being swallowed or falling back to TupleRowParser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8064b11-ebec-435f-928c-f0425296ed7e
📒 Files selected for processing (8)
cassandra/numpy_parser.pyxcassandra/obj_parser.pyxcassandra/parsing.pxdcassandra/parsing.pyxcassandra/protocol.pycassandra/query.pycassandra/row_parser.pyxtests/unit/test_protocol.py
…nt.bind() When Cython serializers (from cassandra.serializers) are available and no column encryption policy is active, BoundStatement.bind() now uses pre-built Serializer objects cached on the PreparedStatement instead of calling cqltype classmethods. This avoids per-value Python method dispatch overhead and enables the ~30x vector serialization speedup from the Cython serializers module. The bind loop is split into three paths: 1. Column encryption policy path (unchanged behavior) 2. Cython serializers path (new fast path) 3. Plain Python path (no CE, no Cython -- removes per-value ColDesc/CE check) Depends on PR scylladb#748 (Cython serializers module) and PR scylladb#630 (CE-policy bind split).
|
Addressing CodeRabbit's review comments (posted as PR comments rather than inline review threads, so replying here): 1. Critical: 2. Nitpick: test bypasses the real 3. Nitpick: duplicated boilerplate (query.py#L687-731, obj_parser.pyx#L69-128) -
All three items verified with the Cython/NumPy extensions built locally ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cassandra/obj_parser.pyx`:
- Around line 69-103: Add a one-time length validation in ParseDesc.__init__
ensuring deserializers has the same number of entries as colnames before
unpack_col_encrypted_row can run. Reject mismatches with a clear assertion or
established validation error, preserving valid ParseDesc construction and
preventing the boundscheck-disabled loop from indexing past deserializers.
In `@tests/unit/test_protocol.py`:
- Line 658: Replace the non-ASCII multiplication symbol in the comments near the
call-count assertions with ASCII wording or an ASCII operator, including both
affected comments. Preserve the comments’ meaning that the call occurs four
times across two rows and two columns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 01123c65-85cd-4b34-a673-8c1516710444
📒 Files selected for processing (7)
cassandra/numpy_parser.pyxcassandra/obj_parser.pyxcassandra/parsing.pxdcassandra/parsing.pyxcassandra/query.pycassandra/row_parser.pyxtests/unit/test_protocol.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/unit/test_protocol.py:804
- This Cython-path test still does not verify the optimization it is intended to protect: the pre-change
ce_policy and ce_policy.contains_column(...)implementation also callscontains_columntwice, so this test would pass before and after the split. Reuse_BoolCountingPolicyhere and assert its truthiness is evaluated once; that will catch a regression back to the per-column check inTupleRowParser.
mock_policy = Mock()
mock_policy.contains_column = Mock(return_value=False)
desc = self._make_parse_desc(column_encryption_policy=mock_policy)
cassandra/query.py:695
- The refactored column-encryption binding branch has no runnable automated coverage. The unit binding tests never configure
column_encryption_policy, while the existing column-encryption integration class is unconditionally skipped, so regressions incontains_column/column_type/encryptbehavior or the one-time truthiness branch will not be detected. Add a focusedBoundStatement.bind()unit test covering both encrypted and unencrypted columns under a policy.
if ce_policy:
def _serialize(value, col_spec):
col_desc = ColDesc(col_spec.keyspace_name, col_spec.table_name, col_spec.name)
if ce_policy.contains_column(col_desc):
col_type = ce_policy.column_type(col_desc)
Split recv_results_rows() into two code paths: one for when column_encryption_policy is set and one for the common case without it. This avoids the per-value 'ce_policy and ce_policy.contains_column(...)' check, which is redundant for the vast majority of queries.
Split TupleRowParser.unpack_row() into unpack_plain_row() and unpack_col_encrypted_row(), branching once in the callers (ListParser, LazyParser, row_parser error path) rather than per value. - Add boundscheck/wraparound Cython optimizations on both methods - Extract try/except outside the per-column loop - NumpyParser.parse_rows() raises NotImplementedError when column encryption is configured, since it has no decryption support - Update parsing.pxd declarations to match
Apply the same split-path optimization to the bind() method, checking for column_encryption_policy once rather than per parameter.
Cover both the pure-Python path (ResultMessage.recv_results_rows) and the Cython fast path (ListParser, NumpyParser) with and without column encryption policy.
…licy When column_encryption_policy is None (the common case), the ColDesc namedtuple list is never accessed by unpack_plain_row(). Skip building it entirely, passing None to ParseDesc instead. This avoids N namedtuple allocations per query (where N = number of columns) on the Cython fast path for non-encrypted workloads.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
cassandra/numpy_parser.pyx:86
- This rejects every NumPy result whenever the cluster has a policy, including queries whose returned columns are all unencrypted. Column-encryption policies are selective (
contains_columndetermines membership), so enabling encryption for one table would now breakNumpyProtocolHandlerqueries against unrelated columns that previously decoded correctly. Reject only result sets containing an encrypted column.
if desc.column_encryption_policy:
raise NotImplementedError(
"NumpyParser does not support column encryption")
cassandra/query.py:695
- The new encryption-policy serialization branch has no active test coverage.
tests/unit/test_parameter_binding.pyextensively coversBoundStatement.bind()only without a policy, while the column-encryption integration suite is class-level skipped. Add unit cases proving an encrypted column usescolumn_type/encrypt, an unencrypted column keeps its schema serializer, and the no-policy path remains unchanged.
if ce_policy:
def _serialize(value, col_spec):
col_desc = ColDesc(col_spec.keyspace_name, col_spec.table_name, col_spec.name)
if ce_policy.contains_column(col_desc):
col_type = ce_policy.column_type(col_desc)
| if len(deserializers) != len(colnames): | ||
| # The row parsers (obj_parser.pyx TupleRowParser.unpack_plain_row / | ||
| # unpack_col_encrypted_row) index into `deserializers` with | ||
| # @cython.boundscheck(False), bounded by rowsize == len(colnames). | ||
| # A length mismatch here would turn into an out-of-bounds memory | ||
| # read at parse time instead of a clean, immediate error, so this | ||
| # invariant is validated once at construction time. Use a real | ||
| # exception (not `assert`) so the guard cannot be stripped by | ||
| # running Python with optimizations enabled (-O). | ||
| raise ValueError( | ||
| "deserializers must have the same length as colnames " | ||
| "(got %d deserializers for %d columns)" % (len(deserializers), len(colnames))) |
Optimize column_encryption_policy checks in the result parsing hot path.
Previously, every decoded value checked
ce_policy and ce_policy.contains_column(coldesc)even when no encryption policy was configured (the common case). This PR splits the parsing into two code paths —unpack_plain_row()andunpack_col_encrypted_row()— and branches once per result set instead of per value.Changes:
recv_results_rows()pure-Python pathTupleRowParser.unpack_row()intounpack_plain_row()/unpack_col_encrypted_row(), with@boundscheck(False)/@wraparound(False)and try/except moved outside the per-column loopcolumn_encryption_policyNumpyParser.parse_rows()raisesNotImplementedErrorwhen column encryption is configured (it has no decryption support)BoundStatement.bind()Benchmark (Cython ListParser, 10k rows, no encryption policy)
Improvement comes from eliminating per-value overhead in the hot loop:
ce_policy and ce_policy.contains_column(coldesc)truthiness checkcoldesc = desc.coldescs[i]lookup (not needed in plain path)try/exceptper value moved outside the loop@cython.boundscheck(False)/@cython.wraparound(False)decoratorsFixes: #582
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.