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

Change Grammar to SinglestoreGrammar #95

Open
bmckay959 opened this issue Jan 30, 2025 · 3 comments
Open

Change Grammar to SinglestoreGrammar #95

bmckay959 opened this issue Jan 30, 2025 · 3 comments
Assignees

Comments

@bmckay959
Copy link
Contributor

I'm not sure if this would be a breaking change for some, so I want to discuss before sending in a PR.

I'm having to build a unique grammar for a binary column and realized that the Grammar class doesn't uniquely identify itself like the other drivers in the ecosystem for laravel. For example, MySqlGrammar, PostgresGrammar, etc.

If we make this change, than anyone using the Grammar class to override or add macros would cause breaking changes. I think this is better for the long term and want to put it on the radar.

Thoughts? I'm happy to make a PR if this is the direction you want to go.

    public function register(): void
    {
        Grammar::macro('typeBinaryUuid', function (Fluent $column) {
            return match (class_basename(static::class)) {
                'MySqlGrammar', 'Grammar' => sprintf('binary(%d)', $column->length ?? 16),
                'PostgresGrammar' => 'bytea',
                'SQLiteGrammar' => 'blob(256)',
                default => throw new \Exception('Unknown Grammar Class')
            };
        });

        Blueprint::macro('binaryUuid', function ($column): ColumnDefinition {
            /** @var \Illuminate\Database\Schema\Blueprint $this */
            return $this->addColumn('binaryUuid', $column);
        });
    }
@AdalbertMemSQL AdalbertMemSQL self-assigned this Jan 30, 2025
@AdalbertMemSQL
Copy link
Collaborator

It sounds like a nice idea.
The same can be done for several more places

use Illuminate\Database\Schema\Grammars\MySqlGrammar;
class Grammar extends MySqlGrammar
use Illuminate\Database\Schema\MySqlBuilder;
class Builder extends MySqlBuilder
use Illuminate\Database\MySqlConnection;
class Connection extends MySqlConnection
use Illuminate\Database\Connectors\MySqlConnector;
class Connector extends MySqlConnector
use Illuminate\Database\Query\Builder as BaseBuilder;
class Builder extends BaseBuilder
use Illuminate\Database\Query\Grammars\MySqlGrammar;
class Grammar extends MySqlGrammar

All these classes can be prefixed with SingleStore.
I think it is a good direction to go. As it is a breaking change - a new version of the connector will be v2.*

@bmckay959
Copy link
Contributor Author

Ok. Sounds good. What should timing be? Or next steps?

I'm happy to PR these changes, but not sure if this alone is worth moving to v2 unless you already have plans for a v2 that this can just be part of?

@AdalbertMemSQL
Copy link
Collaborator

Currently, no new features are planned for the connector, and I’m not sure when there will be. So I’m okay with bumping the version to v2 just for this change. Feel free to submit a PR, and we can move forward with it.

@bmckay959 bmckay959 mentioned this issue Feb 6, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants