Skip to content

Conversation

@FransBouma
Copy link
Contributor

Added a refactoring to FbSchema so the major version number is stored in the object. This then is used to conditionally configure the schema query for the Columns schema so it obtains rdb$relation_fields.rdb$identity_type.
In the returned datatable a new column IS_IDENTITY is added, type bool, which is set to true if the identity_type field in the resultset is 1.

Added a refactoring to FbSchema so the major version number is stored in the object. This then is used to conditionally configure the schema query for the Columns schema so it obtains rdb$relation_fields.rdb$identity_type.
In the returned datatable a new column IS_IDENTITY is added, type bool, which is set to true if the identity_type field in the resultset is 1.
@mrotteveel
Copy link
Member

mrotteveel commented Dec 1, 2020

Firebird 4 introduces RDB$IDENTITY_TYPE = 0 for GENERATED ALWAYS AS IDENTITY (where RDB$IDENTITY_TYPE = 1 is GENERATED BY DEFAULT AS IDENTITY), might be a good idea to take that into account as well; null signifies "not an identity type".

@FransBouma
Copy link
Contributor Author

Is there any documentation regarding what the difference is between generated always and generated by default, so I can also plan on that in my own code ? thanks!

@mrotteveel
Copy link
Member

See Firebird 3 release notes, Identity Column Type and Firebird 4 beta 2 release notes on page 74.

The types are as defined in the SQL:2016 standard. GENERATED BY DEFAULT AS IDENTITY (introduced in Firebird 3) allow you to explicitly assign a value to the identity column (so the value isn't generated, but the explicit value is used), GENERATED ALWAYS AS IDENTITY (introduced in Firebird 4) disallows explicitly assigning a value to the column (the value is always generated). Although this can be overridden with the OVERRIDING clause.

@FransBouma
Copy link
Contributor Author

Thanks. So, at the risk of falling into bikeshedding territory ;), is the 'IS_IDENTITY' column(bool) covering enough? As it loses the information if one could assign a value to the identity column. Might be better perhaps to change it into IDENTITY_TYPE (like it is in the meta-data as well) and set it to NULL if not an identity type and 0 or 1 if it is (so just return the IDENTITY_TYPE column as-is) ?

@mrotteveel
Copy link
Member

I'll leave that decision to Jiri. I don't know if IS_IDENTITY is a 'standard' (or convention) defined column for schemas in the C# world.

For comparison, in JDBC (Java), there is the IS_AUTOINCREMENT metadata column that is defined by the JDBC specification (a tri-state boolean). Jaybird added a non-standard column JB_IDENTITY_TYPE with values ALWAYS, DEFAULT or null.

coll.rdb$collation_name AS COLLATION_NAME,
rfr.rdb$description AS DESCRIPTION
rfr.rdb$description AS DESCRIPTION");
if(this.MajorVersionNumber >= 3)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Missing space after if: if (....

@cincuranet
Copy link
Member

These schema tables vary by provider and are only partly unified. From my POV, having simple IS_IDENTITY is enough. And I think for LLBLGen as well, right @FransBouma?

@FransBouma
Copy link
Contributor Author

For me it's enough to know a field is an identity field, but I can see the benefit for the off chance you'd want to know if the field can be written to or not: IDENTITY_TYPE would cover both situations, while IS_IDENTITY would not. So exposing IDENTITY_TYPE in the view would cover both situations. It's a tiny bit less convenient to use (a nullable int instead of a bool) perhaps ...

I'll make the changes you requested in the review.

@cincuranet
Copy link
Member

cincuranet commented Dec 1, 2020

My vote goes to IS_IDENTITY, for convenience reasons. Knowing what the IDENTITY_TYPE values adds extra need for knowledge and is IMO overkill for most usages.

Also it can be changed later, if some valuable scenario is shown.

@FransBouma
Copy link
Contributor Author

Ok, I'll leave it as IS_IDENTITY and it will be set to true if IDENTITY_TYPE is not null. :)

@FransBouma
Copy link
Contributor Author

I've made the requested changes :)

@cincuranet
Copy link
Member

Few minor nits. I'll merge later today/tomorrow. Thanks!

@FransBouma
Copy link
Contributor Author

Changes made :)

@cincuranet cincuranet merged commit 2c182bf into FirebirdSQL:master Dec 2, 2020
cincuranet added a commit that referenced this pull request Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants