Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years 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's profile ericlewis Owned by: wonderboymusic's profile 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 11 years ago.
add support for precision and scale for decimal
23033.1.patch (776 bytes) - added by ericlewis 11 years ago.
Pervious patch was created in the subdirectory, here's a proper one.
23033.2.diff (600 bytes) - added by ericlewis 11 years ago.
23033.3.diff (599 bytes) - added by ericlewis 11 years ago.
23033.unit-tests.diff (5.6 KB) - added by ericlewis 11 years ago.
23033.4.diff (7.1 KB) - added by ericlewis 11 years ago.

Download all attachments as: .zip

Change History (28)

#1 @SergeyBiryukov
11 years ago

  • Component changed from General to Query

#2 follow-up: @wonderboymusic
11 years 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 11 years ago by wonderboymusic (previous) (diff)

#3 @adamsilverstein
11 years ago

  • Cc ADAMSILVERSTEIN@… added

#4 in reply to: ↑ 2 @knutsp
11 years 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.

#5 @ericlewis
11 years 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
		)
	)
) );

@ericlewis
11 years ago

add support for precision and scale for decimal

#6 @ericlewis
11 years ago

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

@ericlewis
11 years ago

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

#7 @nacin
11 years 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.

#8 @wonderboymusic
11 years ago

#19802 was marked as a duplicate.

#9 follow-up: @wonderboymusic
11 years 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

#10 @dominicp
11 years ago

  • Cc solaveritasteliberum@… added

#11 in reply to: ↑ 9 @ericlewis
11 years 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?

#12 @nacin
11 years ago

  • Milestone changed from 3.7 to Future Release

Moving to future pending unit tests.

@ericlewis
11 years ago

#13 @ericlewis
11 years ago

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

@ericlewis
11 years ago

#15 @ericlewis
11 years 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

#16 follow-up: @nacin
11 years 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.

#17 in reply to: ↑ 16 ; follow-up: @ericlewis
11 years 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 )?

#18 in reply to: ↑ 17 @nacin
11 years 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. :-)

@ericlewis
11 years ago

#19 @ericlewis
11 years ago

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

#20 @ericlewis
11 years ago

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

#21 @wonderboymusic
10 years 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.

#22 @helen
10 years ago

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