Everything

php

Home » Project » Newsgroups » php.internals » [VOTE] PDO Float Type »

[VOTE] PDO Float Type

php.internals

Adam Baratz

9 years ago

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

Adam Baratz

9 years ago

>
> 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

Adam Baratz

9 years ago

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

Adam Baratz

9 years ago

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

Marco Pivetta

9 years ago

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/

On Mon, May 8, 2017 at 3:48 PM, Adam Baratz <adambaratz@php.net> wrote:

Adam Baratz

9 years ago

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

Adam Baratz

9 years ago

>
> 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

Matteo Beccati

9 years ago

Hi Adam,

On 15/05/2017 09:47, Adam Baratz wrote:

> 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/

Adam Baratz

9 years ago

>
> > 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

Matteo Beccati

9 years ago

Hi Adam,

On 15/05/2017 14:54, Adam Baratz wrote:

>> 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/

Adam Baratz

9 years ago

>
> 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

Matteo Beccati

9 years ago

Hi Adam,

On 18/05/2017 14:59, Adam Baratz wrote:

>>
>> 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/