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

Adding support for SingleStore #968

Merged
merged 1 commit into from Apr 24, 2021

Conversation

carlsverre
Copy link
Contributor

Hello! I am an engineer at SingleStore (https://www.singlestore.com/). We build a 99% MySQL compatible database which supports scale out workloads. Unfortunately, we detected a small incompatability between one of our information_schema tables and the corresponding table in MySQL. The specific table is called "parameters". In SingleStore we return a trailing NULL for certain types which causes this connector to fail when it later looks up a type called "int NULL" for example.

I hope this patch can be a starting point to making the parser more robust. I am happy to do more testing if required as a blanket "NULL" replace might not be completely safe here. Possibly if NULL Is the only thing left, then we should keep it, but if it's trailing something else then we should remove it. Would love some suggestions.

Thanks for your time and consideration!

@@ -198,10 +198,16 @@ param4 INTEGER(3)
[InlineData("BINARY(16)", "BINARY", false, 16)]
[InlineData("CHAR(36)", "CHAR", false, 36)]
[InlineData("ENUM('a','b','c')", "ENUM", false, 0)]
[InlineData("NULL", "NULL", false, 0)]
[InlineData("TEXT CHARACTER SET utf8 COLLATE utf8_general_ci", "TEXT", false, 0)]
Copy link
Member

Choose a reason for hiding this comment

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

Should a test for INT NULL (or INT(11) NULL; what does SingleStore use?) be added here?

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 generic NULL test rather than specific ones (see line 209 below)

SingleStore will return INT(11) NULL so the third test on line 186 will test it.

@bgrainger
Copy link
Member

https://www.singlestore.com/free-software/ ends in a form to fill out; is there a simple way to spin up a local instance for testing with Docker?

No, looks like even the Docker Hub image requires a license key via an environment variable: https://hub.docker.com/r/memsql/cluster-in-a-box

May I suggest you make it easier to test SingleStore with just docker run -e ROOT_PASSWORD=pass -p 3306:3306 -p 8080:8080 memsql/cluster-in-a-box (and have it default to using a trial license if one isn't provided).

@carlsverre
Copy link
Contributor Author

That's good feedback and it's something we have gone back and forth about over the years. For now if you don't want to create an account I will happily send you a free edition license which will allow the tests to run no problem. If you want to add it to the repo just use a secret variable for the pipelines if that is ok. You can email me for the license if needed: carl at singlestore dot com.

@carlsverre
Copy link
Contributor Author

Looks like this PR doesn't handle all the possible differences between SingleStore's DTD_IDENTIFIER and MySQL's. We are investigating and will propose a more suitable fix. Is there a test which runs this code against MySQL directly for conformence testing?

@carlsverre
Copy link
Contributor Author

@tavanesov-ua will take this diff over, he is writing a much more robust version of the parser. We should probably also add tests which run directly against SingleStore using our docker container as additional verification. Might do that in a followup PR though. Should have an update here by this time tomorrow.

@bgrainger
Copy link
Member

Is there a test which runs this code against MySQL directly for conformence testing?

It's run indirectly via StoredProcedureTests.

IIRC there's no specific test that tests only SQL parsing for stored procedures against a real MySQL server.

@tavanesov-ua tavanesov-ua force-pushed the fix-singlestore branch 2 times, most recently from 158360c to 6622ceb Compare April 13, 2021 19:58
@tavanesov-ua
Copy link

tavanesov-ua commented Apr 13, 2021

MySQL documentation is not strict about DTD_IDENTIFIER field. It says
The DTD_IDENTIFIER value contains the type name and possibly other information such as the precision or length.

which basically means it can contain additional elements which we add in SingleStore, because we support default values and not null specifier for the function parameters (similarly to the table columns).
The idea is to skip unknown elements and parse only known types. For unknown types, we assume the first element up to space is the type as all MySQL multipart names are already handled here and SingleStore doesn't have multipart data type names.

@carlsverre
Copy link
Contributor Author

Nice job @tavanesov-ua - this looks solid to me. @bgrainger what do you think?

MySQL documentation is not strict about DTD_IDENTIFIER field.
It says: The DTD_IDENTIFIER value contains the type name and
possibly other information such as the precision or length.
So we should not fail on additional elements appearing after
the type and parse only known stuff.

Signed-off-by: Carl Sverre <carl@singlestore.com>
@bgrainger
Copy link
Member

Thanks--I missed that this commit had been rewritten. The new approach looks much more robust (and thanks for the extra tests). I'm concerned about all the extra allocations, but can push some changes to improve that.

@bgrainger bgrainger merged commit 07b149e into mysql-net:master Apr 24, 2021
22 checks passed
@carlsverre
Copy link
Contributor Author

Thanks @bgrainger !

@bgrainger
Copy link
Member

Added in 1.3.7.

@tavanesov-ua
Copy link

@bgrainger thank you!

@tavanesov-ua tavanesov-ua deleted the fix-singlestore branch April 27, 2021 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants