Skip to content

[#7469] improvement(trino-connector): Support catalog configuration directly pass through#7562

Merged
diqiu50 merged 1 commit intoapache:mainfrom
hqbhoho:feat/support_trino_config_bypass_direct
Jul 4, 2025
Merged

[#7469] improvement(trino-connector): Support catalog configuration directly pass through#7562
diqiu50 merged 1 commit intoapache:mainfrom
hqbhoho:feat/support_trino_config_bypass_direct

Conversation

@hqbhoho
Copy link
Contributor

@hqbhoho hqbhoho commented Jul 3, 2025

What changes were proposed in this pull request?

Support directly pass catalog configuration to the Gravitino catalog in the Trino runtime

Why are the changes needed?

PropertyConverter will skip property which not in PropertyConverter#engineToGravitinoMapping, it increases coupling with Trino. We will remove the original logic in the catalog that maps trino.bypass. keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Only remove the trino.bypass. prefix from the configuration key​ and directly pass it through.

FIx: #7469

Does this PR introduce any user-facing change?

no

How was this patch tested?

local tests

@hqbhoho
Copy link
Contributor Author

hqbhoho commented Jul 3, 2025

@diqiu50 @yuqi1129 Could you PTAL at this PR? Thanks!

Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

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

After reviewing the code, I believe we can remove the original logic in the catalog that maps trino.bypass keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Instead, we can adopt the current trino.bypass.direct. mechanism to handle such configurations.

@hqbhoho
Copy link
Contributor Author

hqbhoho commented Jul 3, 2025

After reviewing the code, I believe we can remove the original logic in the catalog that maps trino.bypass keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Instead, we can adopt the current trino.bypass.direct. mechanism to handle such configurations.

okk, I will make adjustments later! That makes the implementation more concise and user-friendly.​

@hqbhoho hqbhoho force-pushed the feat/support_trino_config_bypass_direct branch from b21cd5d to a4eb2e7 Compare July 3, 2025 12:34
@hqbhoho hqbhoho requested a review from diqiu50 July 3, 2025 12:44
@yuqi1129
Copy link
Contributor

yuqi1129 commented Jul 3, 2025

It's a really great refactoring, and no obvious problem was found. @diqiu50 , Could you also take time to take a look?

Copy link
Contributor

@diqiu50 diqiu50 left a comment

Choose a reason for hiding this comment

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

LGTM

@diqiu50 diqiu50 merged commit 10cc598 into apache:main Jul 4, 2025
30 checks passed
sdj3261 pushed a commit to sdj3261/gravitino that referenced this pull request Jul 7, 2025
…tion directly pass through (apache#7562)

### What changes were proposed in this pull request?
Support directly pass catalog configuration to the Gravitino catalog in
the Trino runtime

### Why are the changes needed?

PropertyConverter will skip property which not in
`PropertyConverter#engineToGravitinoMapping`, it increases coupling with
Trino. We will remove the original logic in the catalog that maps
`trino.bypass.` keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Only
remove the `trino.bypass.` prefix from the configuration key​ and
directly pass it through.

FIx: apache#7469 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
local tests
vishnu-chalil pushed a commit to vishnu-chalil/gravitino that referenced this pull request Jul 14, 2025
…tion directly pass through (apache#7562)

### What changes were proposed in this pull request?
Support directly pass catalog configuration to the Gravitino catalog in
the Trino runtime

### Why are the changes needed?

PropertyConverter will skip property which not in
`PropertyConverter#engineToGravitinoMapping`, it increases coupling with
Trino. We will remove the original logic in the catalog that maps
`trino.bypass.` keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Only
remove the `trino.bypass.` prefix from the configuration key​ and
directly pass it through.

FIx: apache#7469 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
local tests
hdygxsj pushed a commit to hdygxsj/gravitino that referenced this pull request Jul 15, 2025
…tion directly pass through (apache#7562)

### What changes were proposed in this pull request?
Support directly pass catalog configuration to the Gravitino catalog in
the Trino runtime

### Why are the changes needed?

PropertyConverter will skip property which not in
`PropertyConverter#engineToGravitinoMapping`, it increases coupling with
Trino. We will remove the original logic in the catalog that maps
`trino.bypass.` keys using the TRINO_KEY_TO_GRAVITINO_KEY map. Only
remove the `trino.bypass.` prefix from the configuration key​ and
directly pass it through.

FIx: apache#7469 

### Does this PR introduce _any_ user-facing change?
no

### How was this patch tested?
local tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] Gravitino Trino connector remove catalog property validation

3 participants