[VOTE] PDO Float Type

7 years ago by Adam Baratz — view source

unread

Hi,

I'd like to open the vote on this RFC:
https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Previous discussion:
https://externals.io/thread/805 https://externals.io/thread/805

Voting will end on the 26th at 0:00 UTC.

Thanks,
Adam

6 years ago by Adam Baratz — view source

unread

I'd like to open the vote on this RFC:
https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Previous discussion:
https://externals.io/thread/805 https://externals.io/thread/805

Voting will end on the 26th at 0:00 UTC.

The RFC was rejected by a vote of 4 to 6. Thanks to all who provided
feedback. I'm going to think some more on this and see if I can be revised
to something more agreeable. Since there were some "no" votes from people
who didn't comment, I'd appreciate hearing if they had any issues that
weren't covered on the list.

Thanks,
Adam

6 years ago by Adam Baratz — view source

unread

Hi,

I took another pass at this RFC:
https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Most of the discussion in the last round was about fixed precision types.
Since there was a lot of disagreement around the right way to handle them,
I'd like to keep them out of this RFC. I know Matteo wanted to get them
into this RFC, but I'd rather do less if it increases the chances that a
group can agree. Nothing in this RFC should interfere with a future
implementation of fixed precision types.

I added a PR to show how this feature would be implemented. It should make
it clear that very little change is needed to open up this functionality. I
avoided mixing in type casts or any special formatting to keep things safe
and simple. This allowed me to streamline the examples in the RFC quite a
bit.

It would be helpful to get feedback from anyone who voted "no" previously,
but didn't comment in the thread. There's no need to talk through this RFC
again if everyone in that group wants fixed precision types to be included.

Thanks,
Adam

6 years ago by Adam Baratz — view source

unread

Hi,

I took another pass at this RFC:

https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Unless discussion prompts major changes, I'll open this for a vote next
Monday.

Thanks,
Adam

6 years ago by Marco Pivetta — view source

unread

Hey Adam,

I voted "no" because for very little improvement (which really isn't worth
it) we get:

a new constant to use
users foot-gunning themselves due to misuse of float values (yes, they
will put floats where real is needed)

That said, see
https://wiki.php.net/rfc/voting#resurrecting_rejected_proposals about
resurrecting the vote: it will need to wait a few months.

Greets,

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Hi,

I took another pass at this RFC:

https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Unless discussion prompts major changes, I'll open this for a vote next
Monday.

Thanks,
Adam

6 years ago by Adam Baratz — view source

unread

Hi,

Thanks for the feedback.

I voted "no" because for very little improvement (which really isn't worth

it)

It would help me to understand why you think this. It's difficult to
generate queries with the best execution plans if the type information
isn't good. Floating point values are part of SQL. Is your thought that
they aren't used enough, that this implementation is missing something, or
something else?

users foot-gunning themselves due to misuse of float values (yes, they

will put floats where real is needed)

Did you see the latest rev of the RFC? It should address this point. If you
pass a string and label it PDO::PARAM_FLT, only emulated prepares and
pdo_sqlite will cast it to a float. And that's only for consistency with
how they handle other types. Does that make you more comfortable? Or is
there another use case you're concerned about?

That said, see https://wiki.php.net/rfc/voting#resurrecting_rejected_propos

als about resurrecting the vote: it will need to wait a few months.

Based on previous feedback, I thought the new version was materially
different. But if a handful of voters from the last round respond and say
that these changes won't change their vote, then sure, there's no point in
rerunning this.

Thanks,
Adam

6 years ago by Adam Baratz — view source

unread

I took another pass at this RFC:

https://wiki.php.net/rfc/pdo_float_type
https://wiki.php.net/rfc/pdo_float_type

Unless discussion prompts major changes, I'll open this for a vote next
Monday.

I had a request on the PR[1] to rename the const, from PDO::PARAM_FLT to
PDO::PARAM_FLOAT. Agree that readability is better here. Since there's been
a change, I'll plan on opening the vote tomorrow.

Thanks,
Adam

[1] https://github.com/php/php-src/pull/2500

6 years ago by Matteo Beccati — view source

unread

Hi Adam,

I had a request on the PR[1] to rename the const, from PDO::PARAM_FLT to
PDO::PARAM_FLOAT. Agree that readability is better here. Since there's been
a change, I'll plan on opening the vote tomorrow.

Are you kidding me?

You can't possibly think that changing the constant name resets the
cooldown counter.

You also said:

Based on previous feedback, I thought the new version was materially
different. But if a handful of voters from the last round respond and say
that these changes won't change their vote, then sure, there's no point in
rerunning this.

You can count Marco and myself as a handful. I see no one else active on
internals saying "yes please".

You insist this change will make wonders but it doesn't. MS SQL
apparently can't use indexes if you pass the float as strings, while
most other databses have evolved since the 2000s and now can.

You could quite easily work around it by embedding your floats in the
SQL command string instead. Which is basically what emulated prepares
would do for you.

On the other hand I am still convinced that the new type could
potentially cause more issues for anyone else not using pdo_dblib.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

6 years ago by Adam Baratz — view source

unread

I had a request on the PR[1] to rename the const, from PDO::PARAM_FLT to
PDO::PARAM_FLOAT. Agree that readability is better here. Since there's
been
a change, I'll plan on opening the vote tomorrow.

Are you kidding me?

You can't possibly think that changing the constant name resets the
cooldown counter.

I don't. I posted a new version of the RFC on May 1st. These are the notes
I sent to this list:

Most of the discussion in the last round was about fixed precision types.

Since there was a lot of disagreement around the right way to handle them,
I'd like to keep them out of this RFC. I know Matteo wanted to get them
into this RFC, but I'd rather do less if it increases the chances that a
group can agree. Nothing in this RFC should interfere with a future
implementation of fixed precision types.

I added a PR to show how this feature would be implemented. It should make

it clear that very little change is needed to open up this functionality. I
avoided mixing in type casts or any special formatting to keep things safe
and simple. This allowed me to streamline the examples in the RFC quite a
bit.

It would be helpful to get feedback from anyone who voted "no" previously,

but didn't comment in the thread. There's no need to talk through this RFC
again if everyone in that group wants fixed precision types to be included.

I think this changes the substance of the proposal, in which case there
wouldn't need to be a cooldown period. It's okay if you disagree, but you
can say so much instead of accusing me of acting in bad faith.

I don't know how my emails come across. I apologize if I seem antagonistic.
I'm not trying to rile you up. I'm just an engineer trying to figure out
how to solve a technical problem. And I'm trying to get a solution that
will please as many people as possible, including you.

You could quite easily work around it by embedding your floats in the

SQL command string instead. Which is basically what emulated prepares
would do for you.

Yes, that would generate the right query, but it could make it difficult to
improve its execution plan in the future.

For example, I'm looking into an approach that would bring real prepared
statements to pdo_dblib:
https://bugs.php.net/bug.php?id=74592

If float parameters aren't bound, each value would result in a new
execution plan, which would negate a lot of the benefit of having prepared
statements.

I kept these kinds of details out of the RFC on purpose. I have my use
cases, but the maintainers of the other drivers should decide how they want
to handle floats. I just want to add the hook so the information is there
if anyone wants to use it.

Thanks,
Adam

6 years ago by Matteo Beccati — view source

unread

Hi Adam,

You can't possibly think that changing the constant name resets the
cooldown counter.

I don't. I posted a new version of the RFC on May 1st. These are the notes
I sent to this list:

Most of the discussion in the last round was about fixed precision types.

Since there was a lot of disagreement around the right way to handle them,
I'd like to keep them out of this RFC. I know Matteo wanted to get them
into this RFC, but I'd rather do less if it increases the chances that a
group can agree. Nothing in this RFC should interfere with a future
implementation of fixed precision types.

If you search the archives, you might find that I wasn't happy to have
PARAM_FLOAT without some kind of PARAM_NUMERIC. You're basically saying
that my point was irrelevant and out of scope. Aww, thanks ;)

I added a PR to show how this feature would be implemented. It should make

it clear that very little change is needed to open up this functionality. I
avoided mixing in type casts or any special formatting to keep things safe
and simple. This allowed me to streamline the examples in the RFC quite a
bit.

The PR could be a 1-liner, but it's the concept behind it that I dislike.

It would be helpful to get feedback from anyone who voted "no" previously,

but didn't comment in the thread. There's no need to talk through this RFC
again if everyone in that group wants fixed precision types to be included.

I think this changes the substance of the proposal, in which case there
wouldn't need to be a cooldown period. It's okay if you disagree, but you
can say so much instead of accusing me of acting in bad faith.

You did get feedback and one of the most prominent PDO users around
seems to agree that it's a fairly bad idea. Do you need more?

He also raised concerns about the cooldown period and you are claiming
that this is a brand new RFC because you've cleaned it up and shoved
some of the biggest concerns into the "out of scope" section.

I don't know how my emails come across. I apologize if I seem antagonistic.
I'm not trying to rile you up. I'm just an engineer trying to figure out
how to solve a technical problem. And I'm trying to get a solution that
will please as many people as possible, including you.

I thank you for your efforts, but, again, out-of-scoping other people's
concerns can't possibly come around as a pleasing solution.

And it's a solution that possibly affects the teeny tiny percentage of
PHP users wanting to optimize queries involving float parameters when
using pdo_dblib. A potential foot-gun for everyone else.

You could quite easily work around it by embedding your floats in the

SQL command string instead. Which is basically what emulated prepares
would do for you.

Yes, that would generate the right query, but it could make it difficult to
improve its execution plan in the future.

For example, I'm looking into an approach that would bring real prepared
statements to pdo_dblib:
https://bugs.php.net/bug.php?id=74592

No offence, but those look like pretty ugly hacks and poor attempts to
overcome what's a huge limitation of the underlying library.

I understand the power of legacy, since I'm running one very legacy
project myself, but please do yourself a favour and help out the
transition away from something as poor as dblib rather than trying to
build your own custom prepared statement emulator. Maybe your skills and
enthusiasm could be helpful to the pdo_sqlsrv team and the driver could
become core at some point?

If float parameters aren't bound, each value would result in a new
execution plan, which would negate a lot of the benefit of having prepared
statements.

Since your driver uses emulated prepares, I'd expect you having a new
execution plan regardless. At least for current master, which is the RFC
target.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/

6 years ago by Adam Baratz — view source

unread

If you search the archives, you might find that I wasn't happy to have
PARAM_FLOAT without some kind of PARAM_NUMERIC. You're basically saying
that my point was irrelevant and out of scope. Aww, thanks ;)

I'm sorry my update sounded like I was ignoring your feedback. Another
change was meant to address your concerns about people misusing
PARAM_FLOAT. Most drivers won't force casts on these values, so if you pass
a string with this type it will work the same as if you used PARAM_STR.
Isn't that the foot-gun you've highlighted?

Maybe we should approach this another way: is there anything that could be
changed with the RFC to change your mind about it? If not, this
conversation is a waste of time for both of us.

For example, I'm looking into an approach that would bring real prepared

statements to pdo_dblib:
https://bugs.php.net/bug.php?id=74592

No offence, but those look like pretty ugly hacks and poor attempts to
overcome what's a huge limitation of the underlying library.

Microsoft actually recommended this solution to my team. We've done only
informal testing, but initial results have been positive.

I understand the power of legacy, since I'm running one very legacy

project myself, but please do yourself a favour and help out the
transition away from something as poor as dblib rather than trying to
build your own custom prepared statement emulator. Maybe your skills and
enthusiasm could be helpful to the pdo_sqlsrv team and the driver could
become core at some point?

I'm doing this to help my team transition off pdo_dblib. Parameters tagged
with the wrong types are going to make it hard for us to migrate code. I'm
considering #74592 as a general improvement, separately, in my capacity as
pdo_dblib's maintainer, since it's not practical for everyone to move off
it.

Since your driver uses emulated prepares, I'd expect you having a new

execution plan regardless. At least for current master, which is the RFC
target.

I meant if the driver is ever changed to one that uses real prepared
statements.

Thanks,
Adam

6 years ago by Matteo Beccati — view source

unread

Hi Adam,

If you search the archives, you might find that I wasn't happy to have
PARAM_FLOAT without some kind of PARAM_NUMERIC. You're basically saying
that my point was irrelevant and out of scope. Aww, thanks ;)

I'm sorry my update sounded like I was ignoring your feedback. Another
change was meant to address your concerns about people misusing
PARAM_FLOAT. Most drivers won't force casts on these values, so if you pass
a string with this type it will work the same as if you used PARAM_STR.
Isn't that the foot-gun you've highlighted?

If you add a parameter that is not going to be used 99% of the time,
then it's even worse.

Maybe we should approach this another way: is there anything that could be
changed with the RFC to change your mind about it? If not, this
conversation is a waste of time for both of us.

As it is now, I'm afraid not.

Cheers

Matteo Beccati

Development & Consulting - http://www.beccati.com/