cast to string in loadByIncrementId, fix big regression for numeric incrementIds

[pepijnblom]

So, we have a client who does not use a character prefix on their incrementId.
Zend auto quotes the value when generating it's queries, but because there's only numbers in their incrementId it does not quote the value.

This is very unfortunate because the difference in speed is quite massive.

Here's the query without quotes around the id. You are seeing that right, that's 30 seconds! There's 2.2 million orders in the system.

time mysql -e "SELECT * FROM sales_flat_order WHERE increment_id=202405845"
real	0m30.788s
user	0m0.000s
sys	0m0.004s

The EXPLAIN:

*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: sales_flat_order
         type: ALL
possible_keys: UNQ_SALES_FLAT_ORDER_INCREMENT_ID,IDX_SALES_FLAT_ORDER_INCREMENT_ID
          key: NULL
      key_len: NULL
          ref: NULL
         rows: 2236262
        Extra: Using where

Now here's the same query with added quotes:

time mysql -e "SELECT * FROM sales_flat_order WHERE increment_id='202405845'"
real	0m0.006s
user	0m0.004s
sys	0m0.000s

The EXPLAIN:

           id: 1
  select_type: SIMPLE
        table: sales_flat_order
         type: const
possible_keys: UNQ_SALES_FLAT_ORDER_INCREMENT_ID,IDX_SALES_FLAT_ORDER_INCREMENT_ID
          key: UNQ_SALES_FLAT_ORDER_INCREMENT_ID
      key_len: 153
          ref: const
         rows: 1
        Extra:

As you can see the database is not using the index because of the mismatch in type.
Percona wrote about this problem in 2006.

Now I've been experimenting a bit and casting to string before calling the method seems to solve the problem already. So I propose to just cast incrementId to string in loadByIncrementId, unless you know of other places this could be changed. We generally notice it with select * queries, which is generally what would roll out of that method.

We are running 10.1.20-MariaDB-1~xenial mariadb.org binary distribution

[Flyingmana]

can you try out the queries with SQL_NO_CACHE please?

[Flyingmana]

forget my last comment.

https://dev.mysql.com/doc/refman/5.7/en/mysql-indexes.html
Comparison of dissimilar columns (comparing a string column to a temporal or numeric column, for example) may prevent use of indexes if values cannot be compared directly without conversion. For a given value such as 1 in the numeric column, it might compare equal to any number of values in the string column such as '1', ' 1', '00001', or '01.e1'. This rules out use of any indexes for the string column.

also a related bug report in MySql which was declined
https://bugs.mysql.com/bug.php?id=83857

So we should really fix this.
Can we maybe fix this in general by basing the fix on the internal DDL cache? (is it even related? I have no idea whats inside the DDL cache)

[LeeSaferite]

Given the fact that increment_id is a text column, I'm 100% on board with casting the parameter to a string as a first step. I agree with @flyingmama that a deeper solution using the DDL cache is better and we should look towards that as well.

[pepijnblom]

@Flyingmana I had a look at the DDL, thanks for pointing me that direction. I traced the DDL to lib/Varien/Db/Adapter/Pdo/Mysql.php

There, a method describeTable is called which collects the DDL and returns it as an array containing all columns of the table. The one for increment_id looks like this:

array(14) {
  ["SCHEMA_NAME"] => NULL
  ["TABLE_NAME"] => string(16) "sales_flat_order"
  ["COLUMN_NAME"] => string(12) "increment_id"
  ["COLUMN_POSITION"] => int(105)
  ["DATA_TYPE"] => string(7) "varchar"
  ["DEFAULT"] => NULL
  ["NULLABLE"] => bool(true)
  ["LENGTH"] => string(2) "50"
  ["SCALE"] => NULL
  ["PRECISION"] => NULL
  ["UNSIGNED"] => NULL
  ["PRIMARY"] => bool(false)
  ["PRIMARY_POSITION"] => NULL
  ["IDENTITY"] => bool(false)
}

So it is retrieving the right data type.

I did some searching for a method quote() which I suspected last time I posted already.
I ended up in \Magento_Db_Adapter_Pdo_Mysql::quote.
The increment_id is passed through it but a type is never passed along.
It then continues to \Zend_Db_Adapter_Abstract::quote but because it still doesn't have a type it gets passed on to \Zend_Db_Adapter_Abstract::_quote

/**
     * Quote a raw string.
     *
     * @param string $value     Raw string
     * @return string           Quoted string
     */
    protected function _quote($value)
    {
        if (is_int($value)) {
            return $value;
        } elseif (is_float($value)) {
            return sprintf('%F', $value);
        }
        return "'" . addcslashes($value, "\000\n\r\\'\"\032") . "'";
    }

Little test:

is_int(201966402) = TRUE // just returns unquoted
is_int('201966402') = FALSE // 
"'" . addcslashes(201966402, "\000\n\r\\'\"\032") . "'" = '201966402'
echo "'" . addcslashes('201966402', "\000\n\r\\'\"\032") . "'" = '201966402'

I can't be bothered to trace it back to where quote() gets called the first time right now, maybe later :)