search

GenieFramework / SearchLight.jl

ORM layer for Genie.jl, the highly productive Julia web framework
https://genieframework.com
MIT License
139 stars 16 forks source link

Integer limit = 4 turn into bigint (4) in postgresql version 10 and 12 #52

Closed VoyagesDivins closed 2 years ago

VoyagesDivins commented 2 years ago

Describe the bug Migration cannot execute on PostgreSQL because "limit = 4" parameter translate into "BIGINT (4)" .

Error stacktrace [error | LibPQ]: SyntaxError: ERROR: syntax error de syntax near « ( » LINE 1: ...ATE TABLE movies (id SERIAL PRIMARY KEY , year BIGINT (4))

To reproduce execute a migration containing column(:year, :integer, limit = 4)

Expected behavior Should create a table with column year of type integer

Additional context

julia> versioninfo() Julia Version 1.7.2 Commit bf53498635 (2022-02-06 15:21 UTC) Platform Info: OS: Windows (x86_64-w64-mingw32) CPU: Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz WORD_SIZE: 64 LIBM: libopenlibm LLVM: libLLVM-12.0.1 (ORCJIT, skylake) Environment: JULIA_REVISE = auto

pkg> st Project Watchtonight v0.1.0 Status D:\devs\tests\Genie\Watchtonight\Project.toml [c43c736e] Genie v4.16.0 [6d011eab] Inflector v1.0.1 [e6f89c97] LoggingExtras v0.4.7 [739be429] MbedTLS v1.0.3 [340e8cb6] SearchLight v2.2.0 [4327cdd6] SearchLightPostgreSQL v2.1.0 [ade2ca70] Dates [56ddb016] Logging

essenciary commented 2 years ago

@VoyagesDivins thanks for this. @michaelfliegner Looks like the last PR caused this: https://github.com/GenieFramework/SearchLightPostgreSQL.jl/pull/10/files Looks like we need to revert that - why did you change it to BIGINT?

michaelfliegner commented 2 years ago

Integer on 64Bit Architecture is BIGINT in POSTGRES. Testing integers for containment in int8ranges for instance requires a cast to BIGINT. I do not want my keys restricted to 32bit.

My change to BIGINT may be the trigger for the problem but imho the cause would rather be a parser quirk that confuses a limit clause with a type cast in the context of not being aware of the BIGINT type. But, of course, the easiest cure is to retract that map to BIGINT.

I'll create a PRQ to restore the clause for :integer and ADD a clause

:bigint => BIGINT,

:integer => :INTEGER,

Regards Michael

On Thu, Mar 24, 2022 at 7:15 PM Adrian Salceanu @.***> wrote:

@VoyagesDivins https://github.com/VoyagesDivins thanks for this. @michaelfliegner https://github.com/michaelfliegner Looks like the last PR caused this: https://github.com/GenieFramework/SearchLightPostgreSQL.jl/pull/10/files Looks like we need to revert that - why did you change it to BIGINT?

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1077910945, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2UVP5666LQ7XOQXUOM3VBSWMPANCNFSM5RR42C2Q . You are receiving this because you were mentioned.Message ID: @.***>

essenciary commented 2 years ago

Sounds like the most flexible approach @michaelfliegner If you're submitting the PR, would you be so kind to also take a look at this please? https://github.com/GenieFramework/SearchLightPostgreSQL.jl/issues/11 Looks like the DATETIME mapping needs changing too. Should take 20s. Thank you

UniqueTokens commented 2 years ago

Related:

VoyagesDivins commented 2 years ago

Not specifying a limit does actually work with BIGINT. Maybe the parser should ignore parameter "limit" when it does not make sense for the db engine ?

michaelfliegner commented 2 years ago

Ok. I'll comply with that. But, again, imho mapping DATETIME to INTEGER reeks of old time weak typing. And Date/Time data types in POSTGRES are date, timestamp, timestamptz and corresponding interval types. However, pragmatically viewed, my packages use only TIMESTAMPTZ and TSTZRANGE and I am not concerned by mapping DATETIME to INTEGER. So I'll retract that one too.

regards Michael

PS: I'll do it NOW

On Fri, Mar 25, 2022 at 8:54 AM Adrian Salceanu @.***> wrote:

Sounds like the most flexible approach @michaelfliegner https://github.com/michaelfliegner If you're submitting the PR, would you be so kind to also take a look at this please? GenieFramework/SearchLightPostgreSQL.jl#11 https://github.com/GenieFramework/SearchLightPostgreSQL.jl/issues/11 Looks like the DATETIME mapping needs changing too. Should take 20s. Thank you

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1078750404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2UUR3E225KINVGXCY5TVBVWKXANCNFSM5RR42C2Q . You are receiving this because you were mentioned.Message ID: @.***>

michaelfliegner commented 2 years ago

Done

On Fri, Mar 25, 2022 at 11:46 AM Michael Fliegner < @.***> wrote:

Ok. I'll comply with that. But, again, imho mapping DATETIME to INTEGER reeks of old time weak typing. And Date/Time data types in POSTGRES are date, timestamp, timestamptz and corresponding interval types. However, pragmatically viewed, my packages use only TIMESTAMPTZ and TSTZRANGE and I am not concerned by mapping DATETIME to INTEGER. So I'll retract that one too.

regards Michael

PS: I'll do it NOW

On Fri, Mar 25, 2022 at 8:54 AM Adrian Salceanu @.***> wrote:

Sounds like the most flexible approach @michaelfliegner https://github.com/michaelfliegner If you're submitting the PR, would you be so kind to also take a look at this please? GenieFramework/SearchLightPostgreSQL.jl#11 https://github.com/GenieFramework/SearchLightPostgreSQL.jl/issues/11 Looks like the DATETIME mapping needs changing too. Should take 20s. Thank you

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1078750404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2UUR3E225KINVGXCY5TVBVWKXANCNFSM5RR42C2Q . You are receiving this because you were mentioned.Message ID: @.***>

michaelfliegner commented 2 years ago

Just for your information: The error in column(:year, :integer, limit = 4) is that the limit keyword is misplaced, what should it even mean? Max four digits? It doesnt work with BIGINT and just the same.not with INTEGER.

The PRQ remains good in my opinion, as s.o. who wants BIGINT, for instance to reference a BIGSERIAL or create a TSTZ8RANGE, should write :bigint in SearchLight.create_table.

regards Michael

Here my reproduction with the corrected TYPEMAPPING:

[ Info: CREATE TABLE testtstzranges (id BIGSERIAL PRIMARY KEY, i TSTZRANGE , year INTEGER (4)) [error | LibPQ]: SyntaxError: ERROR: syntax error at or near "(" LINE 1: ...s (id BIGSERIAL PRIMARY KEY, i TSTZRANGE , year INTEGER (4))

On Fri, Mar 25, 2022 at 12:26 PM Michael Fliegner < @.***> wrote:

Done

On Fri, Mar 25, 2022 at 11:46 AM Michael Fliegner < @.***> wrote:

Ok. I'll comply with that. But, again, imho mapping DATETIME to INTEGER reeks of old time weak typing. And Date/Time data types in POSTGRES are date, timestamp, timestamptz and corresponding interval types. However, pragmatically viewed, my packages use only TIMESTAMPTZ and TSTZRANGE and I am not concerned by mapping DATETIME to INTEGER. So I'll retract that one too.

regards Michael

PS: I'll do it NOW

On Fri, Mar 25, 2022 at 8:54 AM Adrian Salceanu @.***> wrote:

Sounds like the most flexible approach @michaelfliegner https://github.com/michaelfliegner If you're submitting the PR, would you be so kind to also take a look at this please? GenieFramework/SearchLightPostgreSQL.jl#11 https://github.com/GenieFramework/SearchLightPostgreSQL.jl/issues/11 Looks like the DATETIME mapping needs changing too. Should take 20s. Thank you

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1078750404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2UUR3E225KINVGXCY5TVBVWKXANCNFSM5RR42C2Q . You are receiving this because you were mentioned.Message ID: @.***>

michaelfliegner commented 2 years ago

Please check and then close.

VoyagesDivins commented 2 years ago

I did not mention it but I've found that problem when trying to follow the MVC tutorial of Genie Framework using postgresql instead of sqlite.

I think I should have open an issue on Genie repo about documentation.

Solution would be removing "limit = 4" in tutorial or adding a warning about using postgresql.

essenciary commented 2 years ago

@VoyagesDivins - thanks, that makes sense. I need to review, maybe we could ignore limit in Postgres, although I'm not sure if that would work for all types.

michaelfliegner commented 2 years ago

Hello! I'm just nosy. Where is "create table ... limit ..." legal DDL? What I know is "select ... limit ...". Example please?!?

Adrian Salceanu @.***> schrieb am Di., 29. März 2022, 11:50:

@VoyagesDivins https://github.com/VoyagesDivins - thanks, that makes sense. I need to review, maybe we could ignore limit in Postgres, although I'm not sure if that would work for all types.

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1081662780, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2UTSDHUQIFORXDWSWRDVCLG55ANCNFSM5RR42C2Q . You are receiving this because you were mentioned.Message ID: @.***>

essenciary commented 2 years ago

In this case limit is the name of the Julia argument that is used to add a size limit to various types, ex INTEGER(10), VARCHAR(255), CHAR(4), etc

michaelfliegner commented 2 years ago

Varchar and char have a size, but Integer? Is that 10 digits? Or the maximal value? Neither oracle nor POSTGRES have sth. likewise to my knowledge.

essenciary commented 2 years ago

For example MySQL has - 10 digits (used for displaying aka zero padding). However, we don't validate this based on type. If the user passes a non-Nothing value for the limit, it gets sent into the query. We could make it smarter, it order to ignore it where it doesn't make sense.

This is the code for Postgres: 

function SearchLight.Migration.column(name::Union{String,Symbol}, column_type::Union{String,Symbol}, options::Any = ""; default::Any = nothing, limit::Union{Int,Nothing} = nothing, not_null::Bool = false) :: String
  "$name $(TYPE_MAPPINGS[column_type] |> string) " *
    (isa(limit, Int) ? "($limit)" : "") *
    (default === nothing ? "" : " DEFAULT $default ") *
    (not_null ? " NOT NULL " : "") *
    string(options)
end
michaelfliegner commented 2 years ago

Thank You

Adrian Salceanu @.***> schrieb am Di., 29. März 2022, 19:20:

For example MySQL has - 10 digits.

— Reply to this email directly, view it on GitHub https://github.com/GenieFramework/SearchLight.jl/issues/52#issuecomment-1082157101, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6Y2URM7C6GFSIOJS2CM6LVCM3XDANCNFSM5RR42C2Q . You are receiving this because you commented.Message ID: @.***>