Skip to content

DB Information Schema - COLUMNS #318

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

Conversation

jean-bernard-valentaten
Copy link
Contributor

Problem

This is a subset of #264 , covering only COLUMNS and TABLES interfaces.

Different RDBMS return Information Schema (metadata) information in different format. Magento framework should allow to switch between different implementations in order to support those different versions of DBs, as well as differences between versions.

Solution

Introduce granular interfaces, based in Information Schema standard (looking into MySQL and MariaDB definitions in this scope, links are in the document). Application can switch between different implementation based on the profile.

@larsroettig
Copy link
Member

larsroettig commented Oct 15, 2019

@akaplya can you review it pls it looks good to me

@larsroettig
Copy link
Member

Kann be merged told me @akaplya

```

```
class InformationSchema\Table\VarBitColumn extends Column
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to introduce types that are not used by application currently? We will have to make sure that these types support across different databases engines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melnikovi No they are not necessarily needed, I just added them for the sake of being complete. Technically both bit and bit varying have been dropped as of SQL-2003, so we can stimply ommit these.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's omit them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I removed the classes for bit and bit varying from the proposal.

public function getDefaultValue(): ?float {}
}
```
Note that ANSI-SQL defines the exact numerical data types `DECIMAL` and `NUMERIC` as being equivalent. MySQL / MariaDB
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to make columns types generic and delegate responsibility of mapping them to particular database engine to management classes.

Copy link
Contributor Author

@jean-bernard-valentaten jean-bernard-valentaten Oct 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melnikovi Agreed. Should I add a complete overhaul of the DBAL in this proposal?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok to not include this in the design. The idea is that providers will map database specific types to generic types in application.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment in the section "Providers" stating what we have discussed.

/**
* Returns the character set name.
*
* Note that not all RDBMS support this, returning `null` indicates whether this is supported.
Copy link
Member

@melnikovi melnikovi Oct 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't make type like this. We want to be able to swap database engine and have everything work, so API that we provide should be compatible with all databases engines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melnikovi Do you suggest to drop all non-ANSI-SQL features from column classes or do you have something else in mind?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's limit column types to currently used (can check declarative schema to see what is used).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All non-ANSI-SQL features were removed from column classes.

@melnikovi
Copy link
Member

Feedback after review with @akaplya, @buskamuza and @fascinosum

  • Introduce enum for engine. Need to think what MySQL engines do we need to support and create enum based on that
  • Need to remove TYPE from column classes
  • Remove InformationSchema\Table\IntColumn:: getType and create different classes instead

Also need to consider

  • Introducing enums for collation and charset
  • What should be return type of getDefaultValue?
  • What should be return type of getOnUpdate?

@larsroettig
Copy link
Member

larsroettig commented Oct 17, 2019

@melnikovi in PHP we do not have enum type to archive this we need to use a library like https://github.com/spatie/enum or https://docs.spatie.be/enum/v2/introduction/.

@melnikovi
Copy link
Member

@larsroettig I was referring to this https://www.php.net/manual/en/class.splenum.php.

@jean-bernard-valentaten
Copy link
Contributor Author

@melnikovi I have a couple of questions about you considerations:

Introducing enums for collation and charset

Fine with me, but bear in mind that these vary from RDBMS to RDBMS, thus those enums would have to be located in the DBAL.

What should be return type of getDefaultValue?

Can you elaborate what you mean exactly? Imho the return type of getDefaultValue should be tied to the type of column but if you want it to be a generic type, I'd suggest making it ?string. Is that what you are trying to say?

What should be return type of getOnUpdate?

What do you propose?

@jean-bernard-valentaten
Copy link
Contributor Author

Introduce enum for engine. Need to think what MySQL engines do we need to support and create enum based on that

I added that to the proposal. We may not be able to use SplEnum though as it is contained in a PECL package only.

Need to remove TYPE from column classes

I removed them as requested

Remove InformationSchema\Table\IntColumn:: getType and create different classes instead

That was already done, there are type specific classes for all ANSI-SQl variants of integer.

@melnikovi melnikovi self-assigned this Oct 18, 2019
@melnikovi
Copy link
Member

Open questions will be finalized in subsequent proposals.

@melnikovi melnikovi merged commit 369bbf4 into magento:master Oct 18, 2019
@melnikovi melnikovi mentioned this pull request Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants