singlestore-labs / singlestoredb-laravel-driver

The official SingleStore Laravel driver.
https://github.com/singlestore-labs/singlestore-laravel-driver
Apache License 2.0
223 stars 22 forks source link

SingleStore does not allow a order by in a delete query #84

Closed Cosnavel closed 1 month ago

Cosnavel commented 4 months ago

We are in the process of migrating to Singlestore and have encountered a compatibility issue with Laravel's Database Notifications.

The issue arises from the HasDatabaseNotifications trait, which defines a relationship that automatically applies a latest sorting order by default. This sorting behavior is intended for notification retrieval but also inadvertently affects deletion operations. While this automatically generated query functions correctly in MySQL and PostgreSQL, it appears that Singlestore does not support this syntax.

HasDatabaseNotification Trait and Relation ```php morphMany(DatabaseNotification::class, 'notifiable')->latest(); } ```
Example of a produced query when trying to delete a notification via relation and the corresponding error from Singlestore ```sql delete from `notifications` where `notifications`.`notifiable_type` = App \ Models \ User and `notifications`.`notifiable_id` = 218 and `notifications`.`notifiable_id` is not null and JSON_EXTRACT_STRING(data, 'format') = filament and `id` = 9bf43012 - ecff - 4ef1 - afb2 - 26ee0218109f order by `created_at` desc ``` ```sql SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'order by `created_at` desc' at line 1 ```

@carlsverre the behaviour should be documented in Singlestores doc. In the example is a order by in a subquery but no mention of the not allowed behaviour.

Reference: https://docs.singlestore.com/cloud/reference/sql-reference/data-manipulation-language-dml/delete/#example

@olliescase

AdalbertMemSQL commented 4 months ago

Hey @Cosnavel Thanks for pointing out this incompatibility.

With your changes, the connector may produce an incorrect result. For example, when someone wants to delete a single row with the latest timestamp (without order by, it will delete the random row, which will be very unexpected).

I prefer to throw an error in this case by default and add a special flag ignore_order_by_in_delete which can be enabled in the case of migrations when the user knows that order by can be safely ignored.

What do you think about this?

Cosnavel commented 4 months ago

Hey @AdalbertMemSQL, would also be an idea to overwrite the entire handling of the order by in deletes and inject it as a subquery. Would that be possible?

We also encountered several errors because of subqueries like this so in a bit concerned.

General error: 1749 Feature 'Correlated subselect that can not be transformed and does not match on shard keys' is not supported by SingleStore Distributed.
AdalbertMemSQL commented 4 months ago

Hmm.... I think that in some cases (when the table has a unique key) it should be possible. But I'm not sure about the performance of such queries. I need to investigate it deeper. Are you blocked by this feature? I can work on enabling a possibility to ignore order by in deletes and then work on the investigation of implementing this functionality as subselects.

Can you open a separate issue regarding subqueries with examples of queries that caused this error?

arthurmarchesi commented 1 month ago

Hi,

We are going through the same situation. And it's not just DELETE, it also happens with UPDATE when using notifications() from Illuminate\Notifications\Notifiable.

AdalbertMemSQL commented 1 month ago

Looks like this order by behaviour is specific to MySQL

        // When using MySQL, delete statements may contain order by statements and limits
        // so we will compile both of those here. Once we have finished compiling this
        // we will return the completed SQL statement so it will be executed for us.
        if (! empty($query->orders)) {
            $sql .= ' '.$this->compileOrders($query, $query->orders);
        }
    /**
     * Compile an update statement without joins into SQL.
     *
     * @param  \Illuminate\Database\Query\Builder  $query
     * @param  string  $table
     * @param  string  $columns
     * @param  string  $where
     * @return string
     */
    protected function compileUpdateWithoutJoins(Builder $query, $table, $columns, $where)
    {
        $sql = parent::compileUpdateWithoutJoins($query, $table, $columns, $where);

        if (! empty($query->orders)) {
            $sql .= ' '.$this->compileOrders($query, $query->orders);
        }

        if (isset($query->limit)) {
            $sql .= ' '.$this->compileLimit($query, $query->limit);
        }

        return $sql;
    }
arthurmarchesi commented 1 month ago

I see.

The problem is calling the parent::compileUpdateWithoutJoins . It calls the order anyway as SingleStore\Laravel\Query\Grammar extends Illuminate\Database\Query\Grammars\MySqlGrammar

arthurmarchesi commented 1 month ago

Or maybe it will work for updates:

/**
     * Compile an update statement without joins into SQL.
     *
     * @param  \Illuminate\Database\Query\Builder  $query
     * @param  string  $table
     * @param  string  $columns
     * @param  string  $where
     * @return string
     */
    protected function compileUpdateWithoutJoins(Builder $query, $table, $columns, $where)
    {
        $sql = "update {$table} set {$columns} {$where}";

        if (isset($query->limit)) {
            $sql .= ' '.$this->compileLimit($query, $query->limit);
        }

        return $sql;
    }
AdalbertMemSQL commented 1 month ago

With this change, it won't throw an error. But it may delete wrong rows (as it will ignore order by). I checked the code for other drivers and they don't ignore it. So I still stick to the idea that order by in deletes/updates should be disabled by default

arthurmarchesi commented 1 month ago

As we are working with Filament we can't customize the query when using Filament Notifications Database.

Something like these in the Singlestore driver when exists $orders:

UPDATE your_table
SET column = value
WHERE id IN (
    SELECT id
    FROM (
        SELECT id
        FROM your_table
        WHERE your_conditions
        ORDER BY your_order_column
        LIMIT your_limit
    ) AS subquery
);
AdalbertMemSQL commented 1 month ago

Queries like this will only work properly if the table has an id column with unique values.

AdalbertMemSQL commented 1 month ago

I'm planning to add an option to ignore order by. Something like ignore_order_by_in_deletes and ignore_order_by_in_updates. I hope this will help to unblock you.

arthurmarchesi commented 1 month ago

Hi,

To report that Limitis also not supported, but there is this insertion when using updateOrInsert from Illuminate\Database\Query\Builder:

    /**
     * Insert or update a record matching the attributes, and fill it with values.
     *
     * @param  array  $attributes
     * @param  array|callable  $values
     * @return bool
     */
    public function updateOrInsert(array $attributes, array|callable $values = [])
    {
        $exists = $this->where($attributes)->exists();

        if ($values instanceof Closure) {
            $values = $values($exists);
        }

        if (! $exists) {
            return $this->insert(array_merge($attributes, $values));
        }

        if (empty($values)) {
            return true;
        }

        return (bool) $this->limit(1)->update($values);
    }

The exception: PDOException: SQLSTATE[HY000]: General error: 1749 Feature 'UPDATE...LIMIT must be constrained to a single partition' is not supported by SingleStore Distributed.

AdalbertMemSQL commented 1 month ago

@arthurmarchesi Thanks for reporting this case. The limit is supported in some scenarios (when all updated rows are in the same partition). https://docs.singlestore.com/cloud/reference/sql-reference/data-manipulation-language-dml/update/#update-using-limit So, if you filter rows using a shard key, it will work well.

arthurmarchesi commented 1 month ago

Yes, I just checked out. Thank you.