Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing custom param value converters #286

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

filecage
Copy link
Contributor

This PR resolves #283 by implementing a constructor parameter to ParamValueRegistry that allows the default param value converters to be overwritten using array_merge.

This form of implementation is the simplest I could come up with. Personally, I think having some sort of type enum and/or interface to describe a converter closure isn't necessarily the worst idea.

Considerations:

  • Is UnsupportedParamValue meant to be public API? It came in handy for this use case, but it might be desirable to keep this exception class internal.

Todo:

  • Unit Test
  • README update

src/Param/ParamValueConverterRegistry.php Outdated Show resolved Hide resolved
]);

self::assertSame(
'2025-01-28T16:00:00+01:00',
Copy link
Owner

Choose a reason for hiding this comment

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

I'm curious, how do you approach this? ClickHouse does not support this format IIRC

set param_b='2025-01-28T16:00:00+01:00';
select {b: DateTime};

Code: 457. DB::Exception: Value 2025-01-28T16:00:00+01:00 cannot be parsed as DateTime for query parameter 'b' because it isn't parsed completely: only 19 of 25 bytes was parsed: 2025-01-28T16:00:00. (BAD_QUERY_PARAMETER) (version 24.8.6.70 (official build))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry for the confusion. I was under the impression that the date_time_input_format=best_effort setting allows SELECT using ISO 8601 formatted datetime strings, but it does not. It only allows it for INSERT statements (that's where I previously used it).

For SELECT, you would need to use parseDateTimeBestEffort() and its input is a string, not a DateTime. So a working example would be:

set param_b='2025-01-28T16:00:00+01:00';
select parseDateTimeBestEffort({b: String});

I'm sorry for the confusion, the ClickHouse docs aren't necessarily clear when it comes to these details and I've only just started working with CH a couple of weeks ago.

I'll have a look at #287 now and get back to this ASAP

Copy link
Owner

Choose a reason for hiding this comment

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

Ah ok, so it's actually possible to insert ISO8601 compatible value with some clickhouse settings tweaks. Cool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your PR #289 just gave me the right hint that while ISO8601 is not the right choice for my use case, unix timestamps do work:

set param_b=1738150195;
select {b: DateTime};

As they're also timezone-aware, this is at least what I was looking for.

Another thing that led to my confusion is that the output setting date_time_output_format works for SELECT:

set param_b=1738150195;
select {b: DateTime} SETTINGS date_time_output_format='iso';

Copy link
Owner

Choose a reason for hiding this comment

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

Can you mention parseDateTimeBestEffort and date_time_input_format settings as tip/sidenote. IMO it's potentially useful for users.

@simPod simPod force-pushed the #283-customizable-param-registry branch from ee31df2 to 586df5a Compare January 29, 2025 15:15
@simPod simPod merged commit 5c080b3 into simPod:master Jan 29, 2025
16 of 26 checks passed
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.

Allow customization of ParamValueConverterRegistry
2 participants