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

Add support for Sequence flags in meta model #7752

Open
lukaseder opened this issue Aug 10, 2018 · 10 comments

Comments

@lukaseder
Copy link
Member

@lukaseder lukaseder commented Aug 10, 2018

The CREATE SEQUENCE statement supports a few flags that have a representation in many databases. Other flags might simply be ignored. The DDL statement has already been implemented: #7748. Now, we also need access to these flags from the meta model in order to be able to recreate the correct DDL statement from the meta data.

Here are these flags:

  • INCREMENT BY
  • MINVALUE / NO MINVALUE
  • MAXVALUE / NO MAXVALUE
  • START WITH
  • CACHE
  • CYCLE / NO CYCLE

Once we have these flags, they need to be:

  • #8528 interpreted in the DDL interpreter
  • #9327 exported using DSLContext.ddl()
  • #9425 diffed in Meta.diff(Meta)
  • #9438 re-implement ALTER SEQUENCE .. RESTART emulation
  • #9442 code generation support

See also:

@knutwannheden

This comment has been minimized.

Copy link
Contributor

@knutwannheden knutwannheden commented Oct 9, 2019

Looking at the interfaces which describe the information schema (Catalog, Schema, Table, etc.), it looks like the API is mostly in JavaBeans-style (i.e. get-prefixed getters). Thus I would propose to add the following methods to the Sequence interface:

    Number getStartWith();
    Number getIncrementBy();
    Number getMinValue();
    Number getMaxValue();
    Boolean isCycle();
    Number getCache();

@lukaseder: 1) We can replace all Number types by T as we're working with Sequence<T extends Number> 2) We don't need the Boolean wrapper type, see the first question

Trying to devise this API raised a few questions, which I think need to be discussed:

  • Should the API include methods like getNoMinValue() or getNoCycle()?
    These would be necessary if DSLContext#ddl() is expected to call the corresponding CreateSequenceFlagsStep methods, like CreateSequenceFlagsStep#noMinvalue(). I would however rather control this behavior using DDLExportConfiguration.
    Further, if we look a e.g. H2's INFORMATION_SCHEMA.SEQUENCE view, we can see that we there don't have any information either regarding defaulted clauses like NO CYCLE.
    • @lukaseder: I don't think we need this in the meta API. In DDL, NO CYCLE is optional, in case someone wants to make this explicit in DDL. The default is always clear in each database. Perhaps the defaults are not always the same in all dialects, but we can figure this out. Hence, we don't need the boolean wrapper type here
  • Should the methods then even return primitive types?
    For instance isCycle() could return a boolean. IMHO, it still makes sense to use the wrapper types here. A given database may not support the flag and we would require intimate knowledge about every dialect's default values. In H2 for example, the max value appears to default to 9_223_372_036_854_775_807 (2^63 - 1).
    • @lukaseder: We should use primitive type booleans and it is not a problem to discover that "intimate" knowledge of all database's defaults, because we'll be doing that in the integration tests anyway. In most dialects, sequences are BIGINT by default, but again, we allow for any number type through Sequence<T extends Number>, so since we're using generics, we cannot use primitive numeric types here.
  • Using Number as return type could be problematic, since the corresponding fields in CreateSequenceImpl have Field<? extends Number> as their type.
    • @lukaseder: Again, we have a <T extends Number> type in the runtime model.
    • While some dialects like H2 (allow clauses like START WITH 10 + round(rand()) with expressions, the jOOQ parser and other dialects like HSQLDB, MariaDB, Oracle, PostgreSQL, and SQL Server expect constants (i.e. not even 1+1). H2 appears to be exotic in this respect.
      • @lukaseder: I don't think that it is relevant that DDL statements support expressions, because those expressions are evaluated when DDL is executed, and the meta model (dictionary views / information schema) will then contain the respective constant value, right? If not, can you point out some documentation?
    • One solution would be to change the return type Number to match Field<? extends Number> (or Field<T> in the context of Sequence) or possibly it would make more sense to have the DDL interpreter fail processing the statement, if the field isn't an AbstractParam, from which the value can be extracted.
      • @lukaseder: *Not applicable, see various other comments about <T>
    • I would personally favor staying with Number as the return type.
      • @lukaseder: *Not applicable, see various other comments about <T>

@lukaseder What is your take on this?

@knutwannheden

This comment has been minimized.

Copy link
Contributor

@knutwannheden knutwannheden commented Oct 9, 2019

Should the Java code generator also "support" these flags? I.e. generate static Sequence fields into the Sequences class which include this information? If yes, I think we can probably do that as part of a separate issue.

@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 10, 2019

Looking at the interfaces which describe the information schema (Catalog, Schema, Table, etc.), it looks like the API is mostly in JavaBeans-style (i.e. get-prefixed getters).

Yes, correct, that is the historic convention in the jOOQ API, which mixes meta data navigation and DSL:

  • Meta data API is provided using JavaBeans style API (with some exceptions, such as Table.fields(), which should be Table.getFields())
  • DSL API is provided using the most appropriate DSL API method names.

Having said so...

Boolean isCycle();

I think we should use the alternative getCycle() method name, which is possible in JavaBeans as well. This reduces the risk of potential conflict with actual DSL API that uses IS CYCLE as keywords. IS is a bit of a "risky" term in the SQL language.

I'm not sure if we already have a precedent for a boolean value in the meta APIs...

I will comment to your various bullets directly inline, by editing your comment.

Should the Java code generator also "support" these flags? I.e. generate static Sequence fields into the Sequences class which include this information?

Yes of course! All representations of org.jooq.Sequence (regardless where they're from) should support these flags. Imagine recreating the sequences using DSLContext.ddl() based on generated code. Users might want to replicate a PostgreSQL on H2 for testing, based on generated code.

If yes, I think we can probably do that as part of a separate issue.

Yes, sure. A separate issue is fine.

@knutwannheden

This comment has been minimized.

Copy link
Contributor

@knutwannheden knutwannheden commented Oct 11, 2019

So in summary you propose to:

  • Rename isCycle() to getCycle() 👍
  • Change the return type to boolean. This of course implies that CreateSequenceImpl needs to know which of the supported dialects actually recognize the NO CYCLE clause (and for which dialects it must be left out). This probably makes sense anyway (consider translator use case). I will create an issue: #9352
  • Use T as return type for the other methods. Sure, that would also be my preference. But I think you may have missed that CreateSequenceImpl actually supports and internally uses Field rather than Number, which isn't ideal for the interpreter. Here I would like to limit the interpreter's applicability to Param typed values. IMHO it would be best if we could even change to DSL API to require Param<? extends Number> rather than Field<? extends Number>.
@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 11, 2019

This probably makes sense anyway (consider translator use case)

Yes, it makes sense anyway. The CREATE SEQUENCE implementation is not complete, this hasn't been a very conscious omission.

Here I would like to limit the interpreter's applicability to Param typed values

Sure we can do that for now.

IMHO it would be best if we could even change to DSL API to require Param<? extends Number> rather than Field<? extends Number>.

We generally don't do that as there's always one RDBMS that does support arbitrary Field expressions. E.g. we currently do that in the OFFSET .. LIMIT API, but that's not sufficient, because many RDBMS support expressions there, too. We have an issue to fix that, but I cannot find it right now.

We shouldn't limit our main product offering (generic DSL, parser, etc.) because one of our features (interpreter) cannot handle the entirety of possible use cases (yet). We can throw a meaningful error message in case a non-Param is encountered here. Just like we might run into edge cases when interpreting CTAS or view definitions, which we don't entirely understand.

Notice that there are a few other types of Field, which we can still (perhaps) interpret, but which aren't Param. One of them is SQLField, which might make perfect sense for a user to use, because they want to use some literal syntax that we don't support in our DSL, yet.

knutwannheden added a commit that referenced this issue Oct 24, 2019
Adds the following methods to the `Sequence` interface:
`getStartWith()`, `getIncrementBy()`, `getMinValue()`, `getMaxValue()`,
`getCycle()`, and `getCache()`.

The `Sequence` objects returned by the `DDLInterpreter` now return the
expected values when these methods are called. And `DDL` (aka
`DSLContext#ddl()`) queries these methods to create a corresponding
`CREATE SEQUENCE` query.
@knutwannheden

This comment has been minimized.

Copy link
Contributor

@knutwannheden knutwannheden commented Oct 24, 2019

@lukaseder Can you please review 5949f1c?

@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 25, 2019

@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 25, 2019

To prevent merge conflicts, as the diff tool is still quite in motion, I'll apply changes of flags to the diff tool.

@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 25, 2019

In order to implement the diff tool for sequences, I first need

  • Meta.ddl() support for CREATE SEQUENCE statements that include flags (I'd like to write that code only once). Nevermind, @knutwannheden already implemented this. Excellent!
  • ALTER SEQUENCE support (#9437)
@lukaseder

This comment has been minimized.

Copy link
Member Author

@lukaseder lukaseder commented Oct 25, 2019

I've added a new, separate issue for code generation support for these flags: #9442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.