Skip to content

Fix: Resolve PG17 incompatibility for ENUMS in CASE statements #6099

Merged
y0z merged 3 commits intooptuna:masterfrom
vcovo:fix/psql17-enum-trialvalue-type-6096
Jun 19, 2025
Merged

Fix: Resolve PG17 incompatibility for ENUMS in CASE statements #6099
y0z merged 3 commits intooptuna:masterfrom
vcovo:fix/psql17-enum-trialvalue-type-6096

Conversation

@vcovo
Copy link
Copy Markdown
Contributor

@vcovo vcovo commented May 27, 2025

Motivation

I have a postgresql 17.3 DB that I also wanted to use for the results of the trials ran through Optuna - potentially linking them with my own tables through a common id. Unfortunately this lead to an error: operator does not exist: trialvaluetype = character varying error when using PostgreSQL 17.3 as explained in #6096.

Description of the changes

This changes the SQLAlchemy case() structure in TrialModel.find_max/min_value_trial_id to directly compare the ENUM column with Python ENUM members, avoiding problematic VARCHAR casts on string literals.

@HideakiImamura
Copy link
Copy Markdown
Member

@c-bata @gen740 @sawa3030 Could you review this PR?

@gen740
Copy link
Copy Markdown
Member

gen740 commented May 30, 2025

@vcovo
Could you update .github/workflows/tests-storage.yml to include tests for PostgreSQL with psycopg3?
There are already existing tests for psycopg2, so I believe it should be straightforward to update by referring to those lines.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 8, 2025

This pull request has not seen any recent activity.

@github-actions github-actions bot added the stale Exempt from stale bot labeling. label Jun 8, 2025
@github-actions github-actions bot removed the stale Exempt from stale bot labeling. label Jun 9, 2025
@vcovo
Copy link
Copy Markdown
Contributor Author

vcovo commented Jun 11, 2025

@gen740 in case it was missed I added the requested tests. Let me know if anything is still missing for this to be merged.

Comment on lines +201 to +212
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.INF_NEG,
-1,
),
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.FINITE,
0,
),
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.INF_POS,
1,
),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[question] I didn't tested yet, but based on the error message, can we simply write these lines like this?

Suggested change
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.INF_NEG,
-1,
),
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.FINITE,
0,
),
(
TrialValueModel.value_type == TrialValueModel.TrialValueType.INF_POS,
1,
),
{"INF_NEG": -1, "FINITE": 0, "INF_POS": 1},
value=TrialValueModel.value_type.value,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tried several alternatives, but they didn’t work as expected:

  1. Passing .value or .name causes errors, likely because value= expects a column expression rather than an attribute:
case({"INF_NEG": -1, "FINITE": 0, "INF_POS": 1},
     value=TrialValueModel.value_type.value)  # Fails

case({"INF_NEG": -1, "FINITE": 0, "INF_POS": 1},
     value=TrialValueModel.value_type.name)   # Also fails

These cases failed in:
run 15626347340/job 44021240261
run 15624687460/job 44016565482

  1. Using Enum members as dictionary keys also failed:
case(
    {
        TrialValueModel.TrialValueType.INF_NEG: -1,
        TrialValueModel.TrialValueType.FINITE: 0,
        TrialValueModel.TrialValueType.INF_POS: 1,
    },
    value=TrialValueModel.value_type,
)

This failed in:
run 15625311560/job 44018289101

Given these issues, I feel that the current comparison approach is more robust.

@c-bata
Copy link
Copy Markdown
Member

c-bata commented Jun 12, 2025

Let me change the reviewer since I'll be on leave until the week after next.

@y0z Could you review this PR?

@c-bata c-bata assigned y0z and unassigned c-bata Jun 12, 2025
@c-bata c-bata added the bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself. label Jun 12, 2025
Copy link
Copy Markdown
Member

@y0z y0z left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Collaborator

@sawa3030 sawa3030 left a comment

Choose a reason for hiding this comment

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

LGTM

@y0z y0z merged commit 4e3ef39 into optuna:master Jun 19, 2025
14 checks passed
@y0z y0z unassigned y0z and gen740 Jun 19, 2025
@y0z y0z added this to the v4.5.0 milestone Jun 19, 2025
@BarryHart
Copy link
Copy Markdown

Good day!

FYI:
I am using Optuna 4.4.0, postgresql 17.5, and psycopg3 and I got the same issue as above.

In order to fix it, I used psycopg to connect to optuna's database after creating the storage object using optuna.storages.RDBStorage(), then I executed
"cur.execute(
sql.SQL("ALTER TABLE trial_values ALTER COLUMN value_type TYPE VARCHAR;")
)"
on the db before starting the Optuna optimisation.

Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue/PR about behavior that is broken. Not for typos/examples/CI/test but for Optuna itself.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants