laskoviymishka commented on code in PR #13879:
URL: https://github.com/apache/iceberg/pull/13879#discussion_r3684707196
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1051,6 +1051,16 @@ paths:
table. The configuration key "token" is used to pass an access token
to be used as a bearer token
for table requests. Otherwise, a token may be passed using a RFC 8693
token type as a configuration
key. For example, "urn:ietf:params:oauth:token-type:jwt=<JWT-token>".
+
+
+ The response may include a read-restrictions field. A reader that
supports read
Review Comment:
The consumer here is "a reader" throughout — is the intent that this is
specifically an engine that can carry per-principal identity, rather than any
reader?
Enforcing a per-principal decision at query time leans on the engine being
able to pass the requesting principal through to the catalog. Trino fits this
well (and Spark, with a connect-style setup). But simpler engines — ClickHouse,
DuckDB, Postgres — typically integrate with one set of catalog credentials
fixed at config time, have no per-query identity pass-through, and enforce
access with their own in-engine primitives. For those, consuming
read-restrictions isn't a drop-in: it needs identity-mapping infrastructure
that largely doesn't exist yet (I dug into this with the ClickHouse folks
recently, and that was exactly the gap — RR would work, but only after per-user
identity plumbing they don't have today).
That's a fine line to draw and I'm not asking to widen the scope. I just
think it's worth saying explicitly that read-restrictions targets engines that
can carry per-principal identity, so it doesn't get read as a universal
enforcement mechanism every reader can adopt. Is catalog-as-consumer (and,
really, any reader without identity pass-through) deliberately out of scope for
now, or just not spelled out yet?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1051,6 +1051,16 @@ paths:
table. The configuration key "token" is used to pass an access token
to be used as a bearer token
for table requests. Otherwise, a token may be passed using a RFC 8693
token type as a configuration
key. For example, "urn:ietf:params:oauth:token-type:jwt=<JWT-token>".
+
+
+ The response may include a read-restrictions field. A reader that
supports read
+ restrictions must fail any read against the loaded table that cannot
apply a returned
+ restriction in full. This includes unrecognized action or expression
types, and actions
+ whose definition cannot be loaded, parsed, or evaluated. These
restrictions apply to every
+ read performed using this response (including subsequent planTableScan
and fetchScanTasks
+ calls), but subsequent requests or requests made from a different
authentication scope
+ may have different restrictions. The response should not be cached
outside of the
Review Comment:
Was the `should not` here deliberate rather than `MUST NOT`? Since these are
per-principal decisions, my instinct was that caching one principal's
restrictions and reusing them in a different auth scope is exactly what we'd
want to forbid — and every other obligation in this paragraph is already
`must`, so this one reads a little softer than the rest. Is there a case where
caching across auth scopes is intended, or is this just wording?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1051,6 +1051,16 @@ paths:
table. The configuration key "token" is used to pass an access token
to be used as a bearer token
for table requests. Otherwise, a token may be passed using a RFC 8693
token type as a configuration
key. For example, "urn:ietf:params:oauth:token-type:jwt=<JWT-token>".
+
+
+ The response may include a read-restrictions field. A reader that
supports read
+ restrictions must fail any read against the loaded table that cannot
apply a returned
Review Comment:
How is the catalog expected to handle a client that *doesn't* support read
restrictions? This obligation is scoped to "a reader that supports read
restrictions," but a reader that doesn't will just ignore the field and read
unrestricted — so I'm curious whether the assumption is that trust is
pre-established and the catalog 403s otherwise (the model from the dev-list
thread). Is that intended to be stated in the spec, or deliberately left to the
catalog?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3833,6 +3843,317 @@ components:
additionalProperties:
type: string
+ ReadRestrictions:
+ type: object
+ description: >
+ Read restrictions for a table.
+
+ A reader evaluates the row filter against original, untransformed
column
+ values, then applies required-column-projections to the surviving
rows.
+ Each action must produce a value of the same type as the input
column.
+ If a reader that supports read-restrictions cannot apply any returned
+ restriction (a filter expression or an action), it must fail the
query
+ and must not silently return raw, partial, or empty results.
+
+ If a projection targets a nested-typed field (struct, list, or map),
+ other projections in the same ReadRestrictions must not target any
+ nested field-id (struct subfields, list elements, or map keys/values)
+ at any depth. This specification does not define how such actions
+ combine. A reader that receives such a response must fail the query.
+
+ A missing or empty ReadRestrictions object (no
required-column-projections and no
+ required-row-filter) imposes no restrictions.
+ example:
+ required-column-projections:
+ - field-id: 4
+ action: show-last-4
+ - field-id: 6
+ action: replace-with-null
+ - field-id: 8
+ action: truncate-to-year
+ - field-id: 10
+ action: sha-256-global
+ - field-id: 12
+ action: mask-alphanum
+ required-row-filter:
+ type: eq
+ left:
+ type: reference
+ id: 14
+ right:
+ type: literal
+ value: "US"
+ properties:
+ required-column-projections:
+ description: >
+ A list of columns that require specific actions to be applied when
reading.
+ A server must not return an action for a column whose type is not
listed in
+ that action's "Applicable to" set. If absent or empty, no required
actions
+ apply; columns not listed are not subject to any required action.
+
+ 1. For each column listed, the reader must apply the specified
action before
+ returning values for that column.
+
+ 2. The reader must replace all output references to the column
with the result
+ of the action, presenting the result under the original
field-id. For
+ example, if the action for field-id `9` is mask-alphanum, the
reader must
+ return the masked value as field-id `9` in the query output.
+
+ 3. A server must not return more than one projection for the same
field-id
+ in required-column-projections. If a duplicate field-id appears,
the reader
+ must fail the query.
+
+ 4. A projection must not target a map's key field-id. Applying an
action
+ to keys can produce duplicate or null keys, which readers
silently
+ coalesce or reject, causing data loss.
+
+ 5. A reader must enforce projections on the columns it is actually
reading.
+ Projections referencing columns that are not being read do not
apply.
+ type: array
+ items:
+ $ref: '#/components/schemas/Action'
+ required-row-filter:
+ description: >
+ An expression that limits which rows the reader may return.
+
+ 1. The expression must evaluate to a boolean (TRUE or FALSE;
Iceberg predicates
+ never produce NULL). A reader must discard any row for which the
filter
+ evaluates to FALSE, and no information derived from discarded
rows may be
+ included in the query result.
+
+ 2. If this property is absent, null, or always true then no
mandatory filtering is required.
+
+ 3. Column references within the expression must use field IDs
(IdReference),
Review Comment:
The field-id rule makes sense — I'm just not sure what a reader is expected
to do when a field-id referenced in `required-row-filter` isn't present in the
schema it's actually reading (a dropped column, or an older snapshot on
time-travel). Projections have rule 5 for the "not being read" case; is there a
similar fail-closed rule intended for the filter, or am I missing where that's
covered? My worry is that a filter silently no-ops on a missing field-id and
leaks the rows it was meant to hide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
