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

We should not support SQL Server specific syntax or features #231

Open
lauxjpn opened this issue Mar 7, 2024 · 7 comments
Open

We should not support SQL Server specific syntax or features #231

lauxjpn opened this issue Mar 7, 2024 · 7 comments
Milestone

Comments

@lauxjpn
Copy link
Member

lauxjpn commented Mar 7, 2024

For example, the store type varbinary(max) should not be supported. The Jet equivalent would just be a longbinary.

@lauxjpn lauxjpn added the bug label Mar 7, 2024
@lauxjpn lauxjpn added this to the 8.0.0 milestone Mar 7, 2024
@lauxjpn
Copy link
Member Author

lauxjpn commented Apr 13, 2024

@ChrisJollyAU Please link the PR(s) that fixed this to this issue. Thanks!

@ChrisJollyAU
Copy link
Member

Didn't need to make any changes as the issue wasn't an issue.

I believe this got open due to a comment I had with my PR for byte array support #228 .

LENB seems to only work properly if column is a longbinary. If has a max length set column type becomes varbinary(5) (if length is 5). In this case the return value is 6. Is this a length+1 or the closest multiple of 2 (because double byte char set as LEN returns 3)

If that was true then changing it all to longbinary would be required but further investigation showed that my original statement was wrong. longbinary and varchar(max) both work fine and LENB has the same behaviour on both

@lauxjpn
Copy link
Member Author

lauxjpn commented Apr 13, 2024

I am still finding references for varchar(max) and other (max) suffixes for types in the main project. They are then all getting translated to other store types (without (max)):

  • JetByteArrayMethodTranslator
  • JetByteArrayTypeMapping
  • JetStringMethodTranslator
  • JetTypeMappingSource

Jet does not support the (max) suffix that SQL Server uses, so we should not either, unless there is a very good reason for it. If this is because of the SQL Server tests that were used as the original code base for the test suite, then the tests should be changed to be Jet specific, instead of the the provider to satisfy the SQL Server flavor of the tests.

(There are other SQL Server specific code paths as well. For example, the JetModelExtensions class contains sequence related methods though Jet does not support sequences (AFAIK).)

So this issue should be reopened.

@lauxjpn lauxjpn reopened this Apr 13, 2024
@ChrisJollyAU
Copy link
Member

Are you talking about in the _storeTypeMappings variable?

That is only used when scaffolding a database (i.e. database first). The model gets created from JetDatabaseModelFactory and the store type is read from that (acquired in AdoxSchema/DaoSchema in GetDataTypeString

This is the value that is later used to match against the _storeTypeMapping. Naturally as the type is coming from the database itself you will never get varbinary(max)

If you want to do anything, just delete the respective lines.

(There are other SQL Server specific code paths as well. For example, the JetModelExtensions class contains sequence related methods though Jet does not support sequences (AFAIK).)

Correct. There are no sequences in Jet. Effectively they are just a no-op. All they do is set or get the annotation, but nowhere actually uses the annotation value

@lauxjpn lauxjpn changed the title We should not support SQL Server specific syntax We should not support SQL Server specific syntax or features Apr 14, 2024
@lauxjpn
Copy link
Member Author

lauxjpn commented Apr 14, 2024

Right, those kind of references is what this issue is about. All SQL Server specific syntax references or features that are not part of Jet (meaning not actually implemented or working), should be removed from the codebase.

@ChrisJollyAU
Copy link
Member

Not really a bug then. More just code that will never be used. Changed tag to code-cleanup

@lauxjpn
Copy link
Member Author

lauxjpn commented Apr 15, 2024

Sure, that's a first then, but its a good idea to expand the tag collection.

(It's code that should never be used. Currently, it could be used by any user.)

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

No branches or pull requests

2 participants