WordPress.org

Make WordPress Core

Opened 16 months ago

Closed 5 months ago

Last modified 5 months ago

#23033 closed defect (bug) (fixed)

Decimal and numeric options in meta_query do not produce correct MYSQL for floating point numbers comparisons

Reported by: ericlewis Owned by: wonderboymusic
Milestone: 3.8 Priority: normal
Severity: normal Version:
Component: Query Keywords: has-patch needs-refresh has-unit-tests
Focuses: Cc:

Description

If you have a custom post type (shoes) that has floating point numbers (shoe size) as post meta, querying against this post meta with a specific decimal value ( >10.5 ) does not work properly because of the way the values are cast out of the database, and will produce surprising results.

Attachments (6)

23033.patch (740 bytes) - added by ericlewis 15 months ago.
add support for precision and scale for decimal
23033.1.patch (776 bytes) - added by ericlewis 13 months ago.
Pervious patch was created in the subdirectory, here's a proper one.
23033.2.diff (600 bytes) - added by ericlewis 7 months ago.
23033.3.diff (599 bytes) - added by ericlewis 7 months ago.
23033.unit-tests.diff (5.6 KB) - added by ericlewis 7 months ago.
23033.4.diff (7.1 KB) - added by ericlewis 7 months ago.

Download all attachments as: .zip

Change History (28)

comment:1 SergeyBiryukov16 months ago

  • Component changed from General to Query

comment:2 follow-up: wonderboymusic15 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.6
  • Owner set to wonderboymusic
  • Status changed from new to accepted

Our support for CAST needs work. Here is what you have to do:

$stuff = new WP_Query( array(
	'meta_query' => array(
		array(
			'key' => '_edit_last',
			'type' => 'decimal',
			'compare' => '=',
			'value' => 10.5
		)
	)
) );

echo $stuff->request;
// SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  
// INNER JOIN wp_postmeta ON (wp_posts.ID = wp_postmeta.post_id) WHERE 1=1  AND wp_posts.post_type = 'post' 
// AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'private') AND 
// ( (wp_postmeta.meta_key = '_edit_last' AND CAST(wp_postmeta.meta_value AS DECIMAL) = '10.5') ) 
// GROUP BY wp_posts.ID ORDER BY wp_posts.post_date DESC
exit();

We should probably be casting the value (10.5) so the query doesn't end up = (string) '10.5'. We should also support FLOAT.

Last edited 15 months ago by wonderboymusic (previous) (diff)

comment:3 adamsilverstein15 months ago

  • Cc ADAMSILVERSTEIN@… added

comment:4 in reply to: ↑ 2 knutsp15 months ago

  • Cc knut@… added

Replying to wonderboymusic:

We should probably be casting the value (10.5) so the query doesn't end up = (string) '10.5'. We should also support FLOAT.

Cast 10.5 to 10.5 is meaningless. The value is already a float. The problem seems to be that the value is converted to a string when it shouldn't.

comment:5 ericlewis15 months ago

We'll also need to be able to specify the precision and scale for fixed-point data types. I suggest that we use the convention of defining the precision and scale in the 'type' field, and explode the string for parsing when we need to.

$stuff = new WP_Query( array(
	'meta_query' => array(
		array(
			'key' => '_edit_last',
			'type' => 'decimal(3, 1)',
			'compare' => '=',
			'value' => 10.5
		)
	)
) );

ericlewis15 months ago

add support for precision and scale for decimal

comment:6 ericlewis15 months ago

  • Keywords has-patch dev-feedback added; needs-patch removed

ericlewis13 months ago

Pervious patch was created in the subdirectory, here's a proper one.

comment:7 nacin10 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from 3.6 to Future Release

Longstanding issue. Punting to 3.7. Unit tests would be great.

comment:8 wonderboymusic8 months ago

#19802 was marked as a duplicate.

comment:9 follow-up: wonderboymusic8 months ago

  • Keywords needs-refresh added; dev-feedback removed
  • Milestone changed from Future Release to 3.7

#19802 was similar - we also need to fix ORDER logic to CAST properly

comment:10 dominicp8 months ago

  • Cc solaveritasteliberum@… added

comment:11 in reply to: ↑ 9 ericlewis8 months ago

Replying to wonderboymusic:

we also need to fix ORDER logic to CAST properly

This is already covered by the 'orderby' => 'meta_value_num' flag, no?

comment:12 nacin7 months ago

  • Milestone changed from 3.7 to Future Release

Moving to future pending unit tests.

ericlewis7 months ago

comment:13 ericlewis7 months ago

attachment:23033.2.diff is an update for staleness, and also adds in NUMERIC as a supported meta type, which mimics DECIMAL.

ericlewis7 months ago

ericlewis7 months ago

comment:15 ericlewis7 months ago

In attachment:23033.unit-tests.diff, test for:

  • all comparison operators in a WP_Query object with various meta_query settings
  • test_get_cast_for_type() with all the possible meta types

comment:16 follow-up: nacin7 months ago

What happens when I do DECIMAL(a), DECIMAL( 10, 5 ), DECIMAL(a, b), etc? Ideally some basic tests beyond "ANYTHING ELSE" can somewhat demonstrate that this regex works as desired.

Finally, the patches can/should be combined — that way src/ and tests/ can be applied and committed together.

comment:17 in reply to: ↑ 16 ; follow-up: ericlewis7 months ago

Replying to nacin:

What happens when I do DECIMAL(a), DECIMAL( 10, 5 ), DECIMAL(a, b), etc?

How loose of a syntax should we allow for spacing here? Any amount of spaces? DECIMAL( 10 , 5 )?

comment:18 in reply to: ↑ 17 nacin7 months ago

Replying to ericlewis:

Replying to nacin:

What happens when I do DECIMAL(a), DECIMAL( 10, 5 ), DECIMAL(a, b), etc?

How loose of a syntax should we allow for spacing here? Any amount of spaces? DECIMAL( 10 , 5 )?

I don't care if DECIMAL( 10, 5 ) or DECIMAL(10,5) is valid or not, I just wanted a unit test demonstrating them one way or the other. :-)

ericlewis7 months ago

comment:19 ericlewis7 months ago

Sure thing. Added more assertions and combined unit tests and core patch in attachment:23033.4.diff.

comment:20 ericlewis7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

comment:21 wonderboymusic5 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 26055:

Produce proper CAST for DECIMAL and NUMERIC in Meta Query. Adds a bunch of unit tests.

Props ericlewis.
Fixes #23033.

comment:22 helen5 months ago

  • Milestone changed from Future Release to 3.8
Note: See TracTickets for help on using tickets.