Skip to content

Conversation

@TCeason
Copy link
Collaborator

@TCeason TCeason commented Oct 24, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

This commit fixes three critical bugs in row access policy that caused
incorrect filtering behavior with multi-parameter policies.

Issue 1: Parameter case sensitivity (storage)
Parameter names were stored with original case but looked up with
lowercase, causing HashMap mismatches.

Fix: Normalize parameter names to lowercase at creation time to ensure
case-insensitive matching (mirrors masking policy fix).

Issue 2: Parameter case sensitivity (lookup)
When replacing parameters in policy body expressions, the lookup key
used original case while args_map keys were lowercase.

Fix: Normalize lookup keys to lowercase when replacing parameters.

Issue 3: Parameter order not preserved
Using BTreeMap in protobuf serialization sorted parameters alphabetically,
breaking multi-parameter policies where order matters.

Example bug:
CREATE ROW ACCESS POLICY rap AS (UserId, Department) ...
ALTER TABLE t ADD ROW ACCESS POLICY rap ON (c1, c2);

Expected mapping: c1 -> UserId, c2 -> Department Actual mapping: c1 -> Department, c2 -> UserId (sorted!) 

Fix: Add args_v2 field using repeated message to preserve order, with
backward compatibility for old args map field.

Changes:

  • Add RowAccessPolicyArg message and args_v2 field to protobuf
  • Update FromToProto to prioritize args_v2 with fallback to args
  • Normalize parameter names at creation (row_access_policy.rs)
  • Normalize parameter names at lookup (table.rs)
  • Add regression test for multi-parameter mixed-case scenario

Fixes parameter substitution in row access policies.

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - Explain why

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

 This commit fixes three critical bugs in row access policy that caused incorrect filtering behavior with multi-parameter policies. **Issue 1: Parameter case sensitivity (storage)** Parameter names were stored with original case but looked up with lowercase, causing HashMap mismatches. Fix: Normalize parameter names to lowercase at creation time to ensure case-insensitive matching (mirrors masking policy fix). **Issue 2: Parameter case sensitivity (lookup)** When replacing parameters in policy body expressions, the lookup key used original case while args_map keys were lowercase. Fix: Normalize lookup keys to lowercase when replacing parameters. **Issue 3: Parameter order not preserved** Using BTreeMap in protobuf serialization sorted parameters alphabetically, breaking multi-parameter policies where order matters. Example bug: CREATE ROW ACCESS POLICY rap AS (UserId, Department) ... ALTER TABLE t ADD ROW ACCESS POLICY rap ON (c1, c2); Expected mapping: c1 -> UserId, c2 -> Department Actual mapping: c1 -> Department, c2 -> UserId (sorted!) Fix: Add args_v2 field using repeated message to preserve order, with backward compatibility for old args map field. **Changes:** - Add RowAccessPolicyArg message and args_v2 field to protobuf - Update FromToProto to prioritize args_v2 with fallback to args - Normalize parameter names at creation (row_access_policy.rs) - Normalize parameter names at lookup (table.rs) - Add regression test for multi-parameter mixed-case scenario Fixes parameter substitution in row access policies.
@TCeason TCeason requested a review from drmingdrmer as a code owner October 24, 2025 03:35
@github-actions github-actions bot added the pr-bugfix this PR patches a bug in codebase label Oct 24, 2025
@TCeason TCeason marked this pull request as draft October 24, 2025 03:35
@TCeason TCeason marked this pull request as ready for review October 24, 2025 05:51
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

@drmingdrmer reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TCeason)

@TCeason TCeason merged commit 281de99 into databendlabs:main Oct 24, 2025
173 of 180 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix this PR patches a bug in codebase

2 participants