FANNG1 commented on PR #66805:
URL: https://github.com/apache/doris/pull/66805#issuecomment-5311330977
One thing I ran into while implementing this, raising it separately from the
fix itself.
Lance now occupies three flat fields on `TFileScanRangeParams`:
```thrift
36: optional binary lance_substrait_filter
37: optional TExternalSearchRequest external_search_request
38: optional map<string, string> lance_storage_options // this PR
```
Adding the third one is what made me look at the struct as a whole. It is
not a Lance-specific habit — the same shape is already there for several
providers:
```thrift
27: paimon_predicate 30: paimon_options
31: es_properties 32: es_docvalue_context 33:
es_fields_context
34: iceberg_scan_semantics_version
36: hive_parquet_time_zone (master)
```
so this PR follows the prevailing convention rather than departing from it.
But the convention scales badly, and Lance has more coming — index options, a
credential refresh channel (see the note on `lance_storage_options` about
static options), reader mode.
### Why it may be worth doing sooner rather than later
The field ids for these have **already diverged between `branch-4.1` and
`master`**:
| id | `branch-4.1` | `master` |
|---|---|---|
| 31-33 | *(unused)* | `es_properties`, `es_docvalue_context`,
`es_fields_context` |
| 36 | `lance_substrait_filter` | `hive_parquet_time_zone` |
| 37 | `external_search_request` | `lance_substrait_filter` |
| 38 | `lance_storage_options` *(this PR)* | `external_search_request` |
So `38` already means two different things depending on the branch, and
forward-porting this PR to master needs a different id again. Every additional
flat field is one more id to reconcile; a single envelope field would be one.
### Shape
```thrift
struct TLanceScanParams {
1: optional binary substrait_filter
2: optional TExternalSearchRequest external_search_request
3: optional map<string, string> storage_options
}
struct TFileScanRangeParams {
...
N: optional TLanceScanParams lance_scan_params
}
```
`TExternalSearchRequest` stays provider-independent as a type; nesting it
here only says Lance's scanner is what consumes it today, and another provider
can reference the same type from its own params.
Note `TTableFormatFileDesc` already uses this pattern (`paimon_params`,
`iceberg_params`, `lance_params`) — but at *split* level, which is exactly why
credentials were hoisted out of it in this PR. The scan-level struct has no
equivalent.
### The question
Do this here, or as a follow-up?
My inclination is **follow-up**, for two reasons:
- Doing it here means migrating `lance_substrait_filter` and
`external_search_request`, which already exist on the branch. That turns a
credential-loss bug fix into a protocol change touching two shipped fields,
with a dual-write/fallback transition to review alongside it.
- If the goal is a struct that stops growing, Lance is not the only
occupant. Consolidating only Lance while Paimon, ES and Iceberg stay flat
trades one inconsistency for another. A single pass covering all of them seems
more worthwhile than four separate ones.
Happy to do it either way — if the preference is to land it here, say so and
I will fold it in, keeping the old ids reserved rather than reused. Otherwise I
will open an issue for the struct-wide cleanup and link it from here.
--
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]
