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

Feature: Add Real/Float4 type #6467

Closed
wants to merge 4 commits into from
Closed

Feature: Add Real/Float4 type #6467

wants to merge 4 commits into from

Conversation

berkut1
Copy link
Contributor

@berkut1 berkut1 commented Jul 8, 2024

Q A
Type feature
Version 3.9 and 4.1

Summary

Hello,

As I described in this issue #6466 (comment), there is a strange behavior with types in Doctrine, especially after migrating from DC2Type.
One of the problems is that when type comments like DC2Type were used, many checks were seemingly ignored, which allowed the use of the REAL (float4) type. In fact, this is still possible now because there are scattered checks for REAL but they are not fully implemented.

For example, Doctrine won't notice a problem in this case:

CREATE TABLE test_tbl (
   attr REAL NOT NULL, 
);
#[ORM\Table(name: "test_tbl")]
#[ORM\Entity]
class TestEn
{
    private ?int $id = null;
    #[ORM\Column(name: 'attr', type: Types::FLOAT, nullable: false)]
    private float $attr;
}

However, it will try to revert it to DOUBLE PRECISION if we specify DEFAULT, but even here there is a nuance. If we specify it like this, Doctrine will accept the REAL type. (float in quotas in the entity and without them in the migration)

CREATE TABLE test_tbl (
   attr REAL DEFAULT 55.8 NOT NULL, 
);
#[ORM\Column(name: 'attr', type: Types::FLOAT, nullable: false, options: ['default' => '55.8'])]
private float $attr;

Therefore, in this PR, I want to add support for the REAL type, which is supported by most DBMSs, which will fix that weird behavior for float types, and this will also allow people to transition from DC2Type comments without the need to reassign types in mapping_types. It also seems odd to me that Doctrine supports SMALLINT, INT, BIGINT, but only supports float8 and not float4.

Thanks.

@berkut1
Copy link
Contributor Author

berkut1 commented Jul 8, 2024

MySQL, as always, has its own naming conventions; for them, REAL and DOUBLE PRECISION is float8, and FLOAT is float4. I tried to fix this to pass the tests.

UPD:

Because MySQL does things differently from others, the type name REAL might confuse MySQL users. :)

If you are satisfied with the PR, it might be appropriate to rename Types::REAL to Types::SINGLE_FLOAT and accordingly rename the other new functions. What do you think?

P.S
https://mariadb.com/kb/en/float/
https://mariadb.com/kb/en/double/

@berkut1
Copy link
Contributor Author

berkut1 commented Jul 10, 2024

Oracle DBMS also supports this type in its own way :). According to the documentation, the FLOAT type differs only in precision.
https://docs.oracle.com/en/database/oracle/oracle-database/12.2/sqlqr/Data-Types.html
Section: Converting to Oracle Data Types

Checked DBMS:

  • PostgreSQL (represents REAL as float4 or real)
  • MySQL/MariaDB (Feature: Add Real/Float4 type #6467 (comment))
  • MSSQL (represents REAL as real)
  • Oracle (represents REAL as float(63))
  • IBM DB2 (represents REAL as real)
  • SQLite (represents REAL as real)

@derrabus
Copy link
Member

Thank you. Please mind PHPCS.

We currently develop new features on the 4.1 branch. Can you elaborate why you this this change should be merged to 3.9 instead?

@berkut1
Copy link
Contributor Author

berkut1 commented Jul 16, 2024

Oh, because of the existence of the 3.9 branch, I just couldn't decide where to send this new feature first.
Hmm, do I need to recreate this PR for the 4.1 branch only?

@derrabus
Copy link
Member

Unless you can make a very strong case for why we should add this feature to DBAL 3, I'd prefer shipping your feature with DBAL 4 only. No need to recreate the PR, a simple rebase should be enough.

@berkut1
Copy link
Contributor Author

berkut1 commented Jul 16, 2024

Hmm... it was a mistake to make changes in the main branch. :) I tried to isolate my changes in a separate branch and then rebase them onto 4.1, but I'm having issues resolving merge conflicts. The git program doesn't want to accept them, and it's becoming messy. I think it's better if I recreate this PR, if you don't mind.

@greg0ire
Copy link
Member

greg0ire commented Jul 16, 2024

You can build a new branch, reset this branch to the new one and force push. No need for a new PR

@derrabus
Copy link
Member

I think it's better if I recreate this PR, if you don't mind.

Sure, whatever works for you.

@berkut1
Copy link
Contributor Author

berkut1 commented Jul 16, 2024

@greg0ire
I tried, but Git suggests very strange conflict resolutions, deleting files that shouldn't be deleted. It would be faster and safer to recreate the PR.

@berkut1 berkut1 closed this Jul 16, 2024
@berkut1 berkut1 deleted the 3.9.x branch July 16, 2024 11:25
@berkut1 berkut1 mentioned this pull request Jul 16, 2024
6 tasks
derrabus pushed a commit that referenced this pull request Jul 22, 2024
<!-- Fill in the relevant information below to help triage your pull
request. -->

|      Q       |   A
|------------- | -----------
| Type         | feature

#### Summary

Recreated PR #6467 for branch 4.1.

This PR adds support for the `REAL` type for all DBMS, thereby resolving
issues with partial support and potential bugs related to the `REAL`
type in SQL schemas during migration creation. I've tried to explain
this issue in detail through the `INET` type here
#6466.

Checked DBMS:

- [x] PostgreSQL (represents `REAL` as `float4` or `real`)
- [x] MySQL/MariaDB
(#6467 (comment))
- [x] MSSQL (represents `REAL` as `real`)
- [x] Oracle (represents `REAL` as `float(63)`) [doc
](https://docs.oracle.com/en/database/oracle/oracle-database/12.2/sqlqr/Data-Types.html)
- [x] IBM DB2 (represents `REAL` as `real`)
- [x] SQLite (represents `REAL` as `real`)
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.

3 participants