Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Cosnavel opened this issue May 3, 2024 · 14 comments · Fixed by #89
Closed

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

Cosnavel opened this issue May 3, 2024 · 14 comments · Fixed by #89
Assignees

Comments

@Cosnavel
Copy link

Cosnavel commented May 3, 2024

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

namespace Illuminate\Notifications;

trait HasDatabaseNotifications
{
    /**
     * Get the entity's notifications.
     *
     * @return \Illuminate\Database\Eloquent\Relations\MorphMany
     */
    public function notifications()
    {
        return $this->morphMany(DatabaseNotification::class, 'notifiable')->latest();
    }
Example of a produced query when trying to delete a notification via relation and the corresponding error from Singlestore
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
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
Copy link
Collaborator

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
Copy link
Author

Cosnavel commented May 3, 2024

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
Copy link
Collaborator

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?

@AdalbertMemSQL AdalbertMemSQL self-assigned this May 9, 2024
@arthurmarchesi
Copy link

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
Copy link
Collaborator

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

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

arthurmarchesi commented Jul 19, 2024

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
Copy link
Collaborator

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

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
Copy link
Collaborator

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

@AdalbertMemSQL
Copy link
Collaborator

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.

@AdalbertMemSQL AdalbertMemSQL linked a pull request Jul 24, 2024 that will close this issue
@arthurmarchesi
Copy link

Hi,

To report that Limit is 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
Copy link
Collaborator

@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
Copy link

Yes, I just checked out. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants