Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.13.0
    • Fix Version/s: 1.15.0
    • Component/s: core
    • Labels:
      None

      Description

      It would be useful to have the DB version # in the SqlDialect for unparsing

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Resolved in release 1.15.0 (2017-12-11).

          Show
          julianhyde Julian Hyde added a comment - Resolved in release 1.15.0 (2017-12-11).
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/914b5cfb ; thanks for the PR, Christian Beikov !
          Hide
          julianhyde Julian Hyde added a comment -

          I am modifying Christian Beikov's PR so that SqlDialect's preferred constructor takes a single argument - a context object with a "get" method for each property. So, if we add version - or another property - in future we won't have to change the constructors throughout the SqlDialect class hierarchy.

          Show
          julianhyde Julian Hyde added a comment - I am modifying Christian Beikov 's PR so that SqlDialect's preferred constructor takes a single argument - a context object with a "get" method for each property. So, if we add version - or another property - in future we won't have to change the constructors throughout the SqlDialect class hierarchy.
          Hide
          christian.beikov Christian Beikov added a comment -

          The version is passed as constructor argument to every dialect.

          Show
          christian.beikov Christian Beikov added a comment - The version is passed as constructor argument to every dialect.
          Hide
          nerrve Abbas Gadhia added a comment -

          Christian Beikov If I understood correctly, you intend for each dialect to self-configure based on version? However, I noticed that the database version is not an instance variable in SqlDialect. Do you intend to add it later as part of your PR?

          Show
          nerrve Abbas Gadhia added a comment - Christian Beikov If I understood correctly, you intend for each dialect to self-configure based on version? However, I noticed that the database version is not an instance variable in SqlDialect. Do you intend to add it later as part of your PR?
          Hide
          christian.beikov Christian Beikov added a comment -

          Can you review again now please? I updated the PR

          Show
          christian.beikov Christian Beikov added a comment - Can you review again now please? I updated the PR
          Hide
          christian.beikov Christian Beikov added a comment -

          Dependencies to RelDataType and SqlNode are kind of required. I mean we could do it somehow, it's just not practial IMO especially since we already have a dependency on other Sql types because of unparsing of calls.
          I'll try to remove the usage of SqlImplementor.Context and the use of DatabaseMetadata.

          Show
          christian.beikov Christian Beikov added a comment - Dependencies to RelDataType and SqlNode are kind of required. I mean we could do it somehow, it's just not practial IMO especially since we already have a dependency on other Sql types because of unparsing of calls. I'll try to remove the usage of SqlImplementor.Context and the use of DatabaseMetadata.
          Hide
          chris-baynes Chris Baynes added a comment -

          Yup, totally fine with removing Handler. The new way of doing things definitely still fits with the idea of what I wanted - to pull out dialect specific handling code. It's also nice that we can now deal with other dialect specifics; not just operator calls.

          Show
          chris-baynes Chris Baynes added a comment - Yup, totally fine with removing Handler. The new way of doing things definitely still fits with the idea of what I wanted - to pull out dialect specific handling code. It's also nice that we can now deal with other dialect specifics; not just operator calls.
          Hide
          julianhyde Julian Hyde added a comment -

          Christian Beikov, Reviewing https://github.com/apache/calcite/pull/540/commits/e382cabf69cd6171266413a7b25d544522c979b8. A few comments:

          • SqlDialect used to have few dependencies (and this was kind of a goal when I wrote the class), now it has more. Any chance we can reduce the coupling? I.e. remove dependencies on things like SqlImplementor, RelDataType, SqlNode.
          • Thanks for retaining DatabaseProduct in the short term.
          • The things that were annotated Experimental don't need to be marked Deprecated; we can just kill them with no notice.
          • Let's remove the DatabaseMetaData constructor parameter from SqlDialect. DatabaseMetaData contains a Connection, and we don't want the slightest chance that a Dialect will keep that Connection and prevent it from being garbage-collected. Get everything you need from DatabaseMetaData in the factory, before you call the constructor.
          • Before we commit to master, we will need to remove all uses of deprecated APIs (we can mask a few with @SuppressWarnings("deprecation"))
          • Use AvaticaUtils.instantiatePlugin rather than Class.forName. It has a bit more functionality.

          This is a big improvement, and I think we can get this committed pretty soon.

          Chris Baynes, Hope you're ok with Handler being removed. I think that now we have a dialect factory, there are better ways to achieve the same thing. Let me know what you think.

          Show
          julianhyde Julian Hyde added a comment - Christian Beikov , Reviewing https://github.com/apache/calcite/pull/540/commits/e382cabf69cd6171266413a7b25d544522c979b8 . A few comments: SqlDialect used to have few dependencies (and this was kind of a goal when I wrote the class), now it has more. Any chance we can reduce the coupling? I.e. remove dependencies on things like SqlImplementor, RelDataType, SqlNode. Thanks for retaining DatabaseProduct in the short term. The things that were annotated Experimental don't need to be marked Deprecated; we can just kill them with no notice. Let's remove the DatabaseMetaData constructor parameter from SqlDialect. DatabaseMetaData contains a Connection, and we don't want the slightest chance that a Dialect will keep that Connection and prevent it from being garbage-collected. Get everything you need from DatabaseMetaData in the factory, before you call the constructor. Before we commit to master, we will need to remove all uses of deprecated APIs (we can mask a few with @SuppressWarnings("deprecation")) Use AvaticaUtils.instantiatePlugin rather than Class.forName. It has a bit more functionality. This is a big improvement, and I think we can get this committed pretty soon. Chris Baynes , Hope you're ok with Handler being removed. I think that now we have a dialect factory, there are better ways to achieve the same thing. Let me know what you think.
          Hide
          christian.beikov Christian Beikov added a comment -

          Here is the PR for this work: https://github.com/apache/calcite/pull/540

          I wouldn't necessarily create various subtypes for the same product line but have a single one that "self-configures" based on the metadata. Apart from that, I wouldn't rely on instanceof checks because of all the reasons I mentioned so far. Enabling features based on a class hierarchy is just like doing it based on enum value, which IMO is simply not a good idea. If users don't want a specific feature or do want a specific feature that requires the dialect to be a subclass of a specific class they might not be able to achieve that properly when having "conflicting" requirements i.e. having to extend two classes.

          Show
          christian.beikov Christian Beikov added a comment - Here is the PR for this work: https://github.com/apache/calcite/pull/540 I wouldn't necessarily create various subtypes for the same product line but have a single one that "self-configures" based on the metadata. Apart from that, I wouldn't rely on instanceof checks because of all the reasons I mentioned so far. Enabling features based on a class hierarchy is just like doing it based on enum value, which IMO is simply not a good idea. If users don't want a specific feature or do want a specific feature that requires the dialect to be a subclass of a specific class they might not be able to achieve that properly when having "conflicting" requirements i.e. having to extend two classes.
          Hide
          nerrve Abbas Gadhia added a comment -

          I think the names of the classes of the dialects are indicative enough that a particular dialect behaves like a particular product. For example: the class PostgresqlSqlDialect is sufficient to say that it "behaves like Postgres". With this, I think there may not be a need of a special DatabaseProduct enum. I was initially inclined to keep the DatabaseProduct enum, but in retrospect, not anymore.

          If there are multiple dialects that behave like Postgres, then i'm assuming that they will be a part of the same dialect class hierarchy and hence if really must, I could do something like if(dialect instanceof PostgresqlSqlDialect) to figure out if the dialect I have with me has a parent class of PostgresqlSqlDialect. I'm ofc assuming that we are ok having multiple dialects such as Postgresql84SqlDialect and Postgresql96SqlDialect along with the base PostgresqlSqlDialect

          Show
          nerrve Abbas Gadhia added a comment - I think the names of the classes of the dialects are indicative enough that a particular dialect behaves like a particular product. For example: the class PostgresqlSqlDialect is sufficient to say that it "behaves like Postgres". With this, I think there may not be a need of a special DatabaseProduct enum. I was initially inclined to keep the DatabaseProduct enum, but in retrospect, not anymore. If there are multiple dialects that behave like Postgres, then i'm assuming that they will be a part of the same dialect class hierarchy and hence if really must, I could do something like if(dialect instanceof PostgresqlSqlDialect) to figure out if the dialect I have with me has a parent class of PostgresqlSqlDialect. I'm ofc assuming that we are ok having multiple dialects such as Postgresql84SqlDialect and Postgresql96SqlDialect along with the base PostgresqlSqlDialect
          Hide
          christian.beikov Christian Beikov added a comment -

          What does it mean to "behave like Postgres"? For most parts, some forks could be considered to "behave like Postgres" but don't support feature X,Y,Z. Having to introduce additional enum entries(maybe even for various versions of a "product") and to touch switch-code later because of such a wrong assumption is IMO wrong. The mere fact that we expose this vaguely defined thing called "DatabaseProduct" is a problem. People will base assumptions on that, which at a later point might become wrong. Having methods that report capabilities are unambiguous and since we intend to allow users to extend SqlDailect, they can choose to report less, or more capabilities based on their needs. Think about what people might have to do to avoid a single capability to be choosen. Maybe they'd have to return a wrong DatabaseProduct just to get a desired behavior. Also, this stuff is not backwards compatibility friendly. If we choose later to enable the use of a capability for existing DatabaseProducts that users don't want, they have no possibility to revert that rather than forking and building their own calcite version.

          I agree with you that it's generally harder to determine(requires looking through classes), what SqlDialects support a specific feature, but we'd gain the ability to make this extendible.
          Maybe others could comment on what they prefer or think about the matter?

          Show
          christian.beikov Christian Beikov added a comment - What does it mean to "behave like Postgres"? For most parts, some forks could be considered to "behave like Postgres" but don't support feature X,Y,Z. Having to introduce additional enum entries(maybe even for various versions of a "product") and to touch switch-code later because of such a wrong assumption is IMO wrong. The mere fact that we expose this vaguely defined thing called "DatabaseProduct" is a problem. People will base assumptions on that, which at a later point might become wrong. Having methods that report capabilities are unambiguous and since we intend to allow users to extend SqlDailect, they can choose to report less, or more capabilities based on their needs. Think about what people might have to do to avoid a single capability to be choosen. Maybe they'd have to return a wrong DatabaseProduct just to get a desired behavior. Also, this stuff is not backwards compatibility friendly. If we choose later to enable the use of a capability for existing DatabaseProducts that users don't want, they have no possibility to revert that rather than forking and building their own calcite version. I agree with you that it's generally harder to determine(requires looking through classes), what SqlDialects support a specific feature, but we'd gain the ability to make this extendible. Maybe others could comment on what they prefer or think about the matter?
          Hide
          julianhyde Julian Hyde added a comment -

          In http://git-wip-us.apache.org/repos/asf/calcite/commit/67071b6b I have marked SqlDialect.Handler as experimental. This will make it easier to remove after 1.14, if that is what we decide.

          Show
          julianhyde Julian Hyde added a comment - In http://git-wip-us.apache.org/repos/asf/calcite/commit/67071b6b I have marked SqlDialect.Handler as experimental. This will make it easier to remove after 1.14, if that is what we decide.
          Hide
          julianhyde Julian Hyde added a comment -

          See my comment about "ends of the spectrum" on 1st August. Removing DatabaseProduct, as you have done, causes the code to get a lot larger. I want to keep it. We can stop people over-using it by careful review.

          The benefit of DatabaseProduct is concreteness. It's often more concise to say "this database behaves like Postgres in most respects". Conversely, it's not obvious to me what the "supportsNullPrecedence()" method means, although I would probably figure it out if I looked at the point at which it was used.

          Regarding "mix-in interface". I was referring to https://en.wikipedia.org/wiki/Mixin but maybe I wasn't using it correctly. It seems to me that "sequence support" is not separate from "dialect". Some dialects have sequence support, some don't. We can put the utility methods for sequences in the base class, SqlDialect, and if that particular dialect doesn't have sequences then it just won't use them.

          Show
          julianhyde Julian Hyde added a comment - See my comment about "ends of the spectrum" on 1st August. Removing DatabaseProduct, as you have done, causes the code to get a lot larger. I want to keep it. We can stop people over-using it by careful review. The benefit of DatabaseProduct is concreteness. It's often more concise to say "this database behaves like Postgres in most respects". Conversely, it's not obvious to me what the "supportsNullPrecedence()" method means, although I would probably figure it out if I looked at the point at which it was used. Regarding "mix-in interface". I was referring to https://en.wikipedia.org/wiki/Mixin but maybe I wasn't using it correctly. It seems to me that "sequence support" is not separate from "dialect". Some dialects have sequence support, some don't. We can put the utility methods for sequences in the base class, SqlDialect, and if that particular dialect doesn't have sequences then it just won't use them.
          Hide
          christian.beikov Christian Beikov added a comment -

          Julian Hyde I will take it further, this was just a first step into that direction.

          Yes, the functionality of SqlDialect.Handler is moved directly into SqlDialect. Code from the BaseHandler is directly in SqlDialect and the other handlers are inlined into their respective dialects. I didn't know since when the Handler existed so I tried to mark things as deprecated if any consumer of Calcite was using these classes.

          I'd like to remove DatabaseProduct alltogether. In my opinion, making an enum publicly accessible like that, has no value. Would it be so bad to replace code like if (dialect.getDatabaseProduct() == SqlDialect.DatabaseProduct.MYSQL) with if (!dialect.supportsNullPrecedence()? The implementation of the MysqlDialect would simply return false for that capability. I don't think this is too bad from a readability point of view and performance wise it's probably also better. If we encapsulate these capability checks all through methods on SqlDialect so that users can alter that behavior, what do you think the DatabaseProduct would be useful then for? Except for misusing it, I can't think of it being of any value.

          What do you mean by "mix-in interface"? Would you propose using the standard information schema implementation as a base implementation in SqlDialect and then let subclasses override that? That could work too but I'm not sure if that design is better. What is it that you would like to avoid? The additional SequenceSupport object on which I call methods? Or the classes related to sequence support? I guess I could put the resolving of the concrete sequence support directly into the dialects and even inline the implementation of SequenceSupportImpl if that is what you are getting at.

          As I wrote before, this PR is far from being done, it's just a first cut. Anyway, I hope you are beginning to like the idea and we can move this forward

          Show
          christian.beikov Christian Beikov added a comment - Julian Hyde I will take it further, this was just a first step into that direction. Yes, the functionality of SqlDialect.Handler is moved directly into SqlDialect . Code from the BaseHandler is directly in SqlDialect and the other handlers are inlined into their respective dialects. I didn't know since when the Handler existed so I tried to mark things as deprecated if any consumer of Calcite was using these classes. I'd like to remove DatabaseProduct alltogether. In my opinion, making an enum publicly accessible like that, has no value. Would it be so bad to replace code like if (dialect.getDatabaseProduct() == SqlDialect.DatabaseProduct.MYSQL) with if (!dialect.supportsNullPrecedence() ? The implementation of the MysqlDialect would simply return false for that capability. I don't think this is too bad from a readability point of view and performance wise it's probably also better. If we encapsulate these capability checks all through methods on SqlDialect so that users can alter that behavior, what do you think the DatabaseProduct would be useful then for? Except for misusing it, I can't think of it being of any value. What do you mean by "mix-in interface"? Would you propose using the standard information schema implementation as a base implementation in SqlDialect and then let subclasses override that? That could work too but I'm not sure if that design is better. What is it that you would like to avoid? The additional SequenceSupport object on which I call methods? Or the classes related to sequence support? I guess I could put the resolving of the concrete sequence support directly into the dialects and even inline the implementation of SequenceSupportImpl if that is what you are getting at. As I wrote before, this PR is far from being done, it's just a first cut. Anyway, I hope you are beginning to like the idea and we can move this forward
          Hide
          julianhyde Julian Hyde added a comment -

          I like the idea of SqlDialectFactory. In PR 514 you need to take it a bit further - add a connect string property dialectFactory so that people can provide their own.

          Once dialects can be loaded as plug-ins, the case for SqlDialect.Handler is weaker. I saw that you marked it deprecated, i.e. you intend to remove it. Do you intend to replace it with a sub-class of SqlDialect?

          Chris Baynes, I would like to know what you think, since you just added SqlDialect.Handler.

          If we will probably remove Handler, we should mark it experimental (with the annotation org.apache.calcite.linq4j.function.Experimental) right now, before release 1.14. Then we won't need to go through the deprecation process.

          Regarding DatabaseProduct. I do not believe that every dialect should have its own product: it's OK for a dialect to have product = UNKNOWN, and it's also OK to have two dialects that both have product = MYSQL. So, DatabaseProduct is for the common 80%, not for 100%. I think we should keep DatabaseProduct, but sub-class SqlDialect to achieve behavior that is finer-grained than DatabaseProduct allows.

          Regarding SequenceSupport in PR 514. I think it should be a mix-in interface that extends SqlDialect. Then SequenceSupportImpl will merge into SqlDialect, and its several sub-classes will merge into their respective dialect sub-classes.

          Show
          julianhyde Julian Hyde added a comment - I like the idea of SqlDialectFactory . In PR 514 you need to take it a bit further - add a connect string property dialectFactory so that people can provide their own. Once dialects can be loaded as plug-ins, the case for SqlDialect.Handler is weaker. I saw that you marked it deprecated, i.e. you intend to remove it. Do you intend to replace it with a sub-class of SqlDialect? Chris Baynes , I would like to know what you think, since you just added SqlDialect.Handler . If we will probably remove Handler, we should mark it experimental (with the annotation org.apache.calcite.linq4j.function.Experimental) right now, before release 1.14. Then we won't need to go through the deprecation process. Regarding DatabaseProduct. I do not believe that every dialect should have its own product: it's OK for a dialect to have product = UNKNOWN, and it's also OK to have two dialects that both have product = MYSQL. So, DatabaseProduct is for the common 80%, not for 100%. I think we should keep DatabaseProduct, but sub-class SqlDialect to achieve behavior that is finer-grained than DatabaseProduct allows. Regarding SequenceSupport in PR 514. I think it should be a mix-in interface that extends SqlDialect. Then SequenceSupportImpl will merge into SqlDialect, and its several sub-classes will merge into their respective dialect sub-classes.
          Hide
          christian.beikov Christian Beikov added a comment -

          I implemented part of what you describe as part of a PR that adds sequence support. Maybe you could comment on that? Jess Balint
          https://github.com/apache/calcite/pull/514

          By allowing a user to provide a custom SqlDialectFactory, a user could make use of a custom dialect implementation. That way, a consumer of calcite can overrule if a feature is used or not. Still, the default implementation can make a decision about whether a feature is used by doing version comparisons. That obviously requires that SqlDialect is the "gate-keeper" to what a dialect actually supports rather than handling that through checks against DatabaseProduct.
          My prime example for when the DatabaseProduct approach will break is MySQL. With version 8 it will support CTEs thus we could push down such SQL, yet we only have a single MYSQL enum entry. We could introduce multiple entries for respective versions(e.g. MYSQL5, MYSQL8) to handle such cases, but that won't handle cases where a user might not want to use a specific feature.

          Allowing SqlDialect to handle unparsing or return whether a DBMS has a specific capability also allows for adding new dialects that don't fit into the current DatabaseProduct scheme. What if there is a MySQL fork out there that doesn't support feature X,Y,Z of our "standard MySQL" version we defined through a DatabaseProduct? These people would be out of luck right now.

          Maybe you (Julian Hyde) could comment further about what he doesn't like about this approach so we could tackle specifics?

          Show
          christian.beikov Christian Beikov added a comment - I implemented part of what you describe as part of a PR that adds sequence support. Maybe you could comment on that? Jess Balint https://github.com/apache/calcite/pull/514 By allowing a user to provide a custom SqlDialectFactory, a user could make use of a custom dialect implementation. That way, a consumer of calcite can overrule if a feature is used or not. Still, the default implementation can make a decision about whether a feature is used by doing version comparisons. That obviously requires that SqlDialect is the "gate-keeper" to what a dialect actually supports rather than handling that through checks against DatabaseProduct. My prime example for when the DatabaseProduct approach will break is MySQL. With version 8 it will support CTEs thus we could push down such SQL, yet we only have a single MYSQL enum entry. We could introduce multiple entries for respective versions(e.g. MYSQL5, MYSQL8) to handle such cases, but that won't handle cases where a user might not want to use a specific feature. Allowing SqlDialect to handle unparsing or return whether a DBMS has a specific capability also allows for adding new dialects that don't fit into the current DatabaseProduct scheme. What if there is a MySQL fork out there that doesn't support feature X,Y,Z of our "standard MySQL" version we defined through a DatabaseProduct? These people would be out of luck right now. Maybe you ( Julian Hyde ) could comment further about what he doesn't like about this approach so we could tackle specifics?
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          Removed version

          Show
          jbalint@gmail.com Jess Balint added a comment - Removed version
          Hide
          julianhyde Julian Hyde added a comment -

          Jess Balint, Do you plan to work on this for 1.14? If not, that's fine, but please clear the "fix version" field.

          Show
          julianhyde Julian Hyde added a comment - Jess Balint , Do you plan to work on this for 1.14? If not, that's fine, but please clear the "fix version" field.
          Hide
          julianhyde Julian Hyde added a comment -

          Use your discretion and come up with a pull request.

          Show
          julianhyde Julian Hyde added a comment - Use your discretion and come up with a pull request.
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          What would you suggest at this point?

          Show
          jbalint@gmail.com Jess Balint added a comment - What would you suggest at this point?
          Hide
          julianhyde Julian Hyde added a comment -

          Horses for courses, as they say. It depends.

          For FETCH, I would add a method to SqlDialect. In fact it is a perfect example of that. One indication of this is that the method can be concisely described in javadoc. The application shouldn't have to worry about versions, just capabilities.

          Regarding JdbcRules. Determining what can be pushed down is extremely complicated. Not just based on SqlKind, or SqlOperator, but sometimes based on the types of the arguments to the operator. I wouldn't attempt to apply to the same policy to that as to SQL generation.

          Regarding n-ary CONCAT. This does seem a case for adding version as a protected field (with no accessor method) in SqlDialect.

          Note that static DatabaseProduct SqlDialect.getProduct(String productName, String productVersion) has a productVersion argument (not used). So we could even include version in DatabaseProduct. But we'd have to live with the consequences.

          Show
          julianhyde Julian Hyde added a comment - Horses for courses, as they say. It depends. For FETCH , I would add a method to SqlDialect . In fact it is a perfect example of that. One indication of this is that the method can be concisely described in javadoc. The application shouldn't have to worry about versions, just capabilities. Regarding JdbcRules . Determining what can be pushed down is extremely complicated. Not just based on SqlKind , or SqlOperator , but sometimes based on the types of the arguments to the operator. I wouldn't attempt to apply to the same policy to that as to SQL generation. Regarding n-ary CONCAT . This does seem a case for adding version as a protected field (with no accessor method) in SqlDialect . Note that static DatabaseProduct SqlDialect.getProduct(String productName, String productVersion) has a productVersion argument (not used). So we could even include version in DatabaseProduct . But we'd have to live with the consequences.
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          People may choose not to use a feature if it's buggy? This would mean dialects are not only a reflection of the SQL dialect, but also user preferences?

          So let's look at some examples here.

          First, we see JdbcRules maintaining two sets of aggregates:

            static final ImmutableList<SqlKind> AGG_FUNCS;
            static final ImmutableList<SqlKind> MYSQL_AGG_FUNCS;
          

          and choosing between these based on DB product:

              switch (sqlDialect.getDatabaseProduct()) {
              case MYSQL:
                return MYSQL_AGG_FUNCS.contains(aggregation.getKind());
              default:
                return AGG_FUNCS.contains(aggregation.getKind());
              }
          

          Sure, we can encapsulate the set of aggregate functions behind the SqlDialect API and a add a subclass for MySQL (version-independent). Would it make sense here? I don't have a preference.

          Something like SqlImplementor.rewriteSingleValueExpr() uses:

              switch (sqlDialect.getDatabaseProduct()) {
              case MYSQL:
              case HSQLDB:
          

          here, we can encapsulate this with either a boolean supportsSingleValueRewrite() method or adding rewriteSingleValueExpr() to the dialect.

          However, in my application, I have something very similar to test for support of FETCH. SQL Server 2008 doesn't support it. I can either ask for the version from the dialect:

          		switch (dialect.getDatabaseProduct()) {
          			case MSSQL:
          				return dialect.getDatabaseMajorVersion() >= 11;
          

          or add a new method to SqlDialect such as supportsFetch() (with different semantics than the current supportsOffsetFetch())). Version is also required when deciding how to translate functions, e.g. SQL Server 2012 supports n-ary CONCAT() but SQL Server 2008 needs to use +. If SqlDialect doesn't expose version, then what? Add supportsNaryConcat() or have each dialect maintain it's own operator table?

          Show
          jbalint@gmail.com Jess Balint added a comment - People may choose not to use a feature if it's buggy? This would mean dialects are not only a reflection of the SQL dialect, but also user preferences? So let's look at some examples here. First, we see JdbcRules maintaining two sets of aggregates: static final ImmutableList<SqlKind> AGG_FUNCS; static final ImmutableList<SqlKind> MYSQL_AGG_FUNCS; and choosing between these based on DB product: switch (sqlDialect.getDatabaseProduct()) { case MYSQL: return MYSQL_AGG_FUNCS.contains(aggregation.getKind()); default : return AGG_FUNCS.contains(aggregation.getKind()); } Sure, we can encapsulate the set of aggregate functions behind the SqlDialect API and a add a subclass for MySQL (version-independent). Would it make sense here? I don't have a preference. Something like SqlImplementor.rewriteSingleValueExpr() uses: switch (sqlDialect.getDatabaseProduct()) { case MYSQL: case HSQLDB: here, we can encapsulate this with either a boolean supportsSingleValueRewrite() method or adding rewriteSingleValueExpr() to the dialect. However, in my application, I have something very similar to test for support of FETCH . SQL Server 2008 doesn't support it. I can either ask for the version from the dialect: switch (dialect.getDatabaseProduct()) { case MSSQL: return dialect.getDatabaseMajorVersion() >= 11; or add a new method to SqlDialect such as supportsFetch() (with different semantics than the current supportsOffsetFetch() )). Version is also required when deciding how to translate functions, e.g. SQL Server 2012 supports n-ary CONCAT() but SQL Server 2008 needs to use + . If SqlDialect doesn't expose version, then what? Add supportsNaryConcat() or have each dialect maintain it's own operator table?
          Hide
          julianhyde Julian Hyde added a comment -

          I think we should strike a balance between putting the logic outside the dialect (e.g. switch statements in SqlImplementor) and inside the dialect (i.e. methods in SqlDialect that can be overridden in sub-classes). Both ends of the spectrum can lead to ridiculous code.

          I'm not 100% against adding version to SqlDialect. But it would make it easier to over-use switch statements.

          And by the way, a particular feature might be introduced in database version X but people might choose not to use it until version Y. Depending too much on versions would make it difficult to deal with that case.

          Show
          julianhyde Julian Hyde added a comment - I think we should strike a balance between putting the logic outside the dialect (e.g. switch statements in SqlImplementor) and inside the dialect (i.e. methods in SqlDialect that can be overridden in sub-classes). Both ends of the spectrum can lead to ridiculous code. I'm not 100% against adding version to SqlDialect. But it would make it easier to over-use switch statements. And by the way, a particular feature might be introduced in database version X but people might choose not to use it until version Y. Depending too much on versions would make it difficult to deal with that case.
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          Would you rather replace all uses of getDatabaseProduct() in, say, SqlImplementor by calls to the dialect where the logic resides? This means we either put more switch statements in SqlDialect or create subclasses to specialize behavior. I suggested adding subclasses for CALCITE-1841. If you prefer this route, I will remove the db product accessor and hide all behavior switches behind the SqlDialect API.

          Show
          jbalint@gmail.com Jess Balint added a comment - Would you rather replace all uses of getDatabaseProduct() in, say, SqlImplementor by calls to the dialect where the logic resides? This means we either put more switch statements in SqlDialect or create subclasses to specialize behavior. I suggested adding subclasses for CALCITE-1841 . If you prefer this route, I will remove the db product accessor and hide all behavior switches behind the SqlDialect API.
          Hide
          julianhyde Julian Hyde added a comment -

          Yep, exposing the DB product was an intentional leak in the abstraction. For some things, it's just more straightforward to have an enum to put in a switch statement. But that doesn't mean I'm OK widening the leak to a gaping hole.

          Show
          julianhyde Julian Hyde added a comment - Yep, exposing the DB product was an intentional leak in the abstraction. For some things, it's just more straightforward to have an enum to put in a switch statement. But that doesn't mean I'm OK widening the leak to a gaping hole.
          Hide
          jbalint@gmail.com Jess Balint added a comment -

          You could argue the same for exposing the DB product from the SqlDialect. The low-level details are exposed for others - whether it's a CALCITE-1841 Handler, or other (3rd party) code - to build those abstractions. This doesn't prevent us from adding more methods to a dialect to determine behavior (e.g. the existing supportsOffsetFetch(), allowsAs(), requiresAliasForFromItems(), etc).

          Show
          jbalint@gmail.com Jess Balint added a comment - You could argue the same for exposing the DB product from the SqlDialect . The low-level details are exposed for others - whether it's a CALCITE-1841 Handler , or other (3rd party) code - to build those abstractions. This doesn't prevent us from adding more methods to a dialect to determine behavior (e.g. the existing supportsOffsetFetch() , allowsAs() , requiresAliasForFromItems() , etc).
          Hide
          julianhyde Julian Hyde added a comment -

          I'd rather judge a SqlDialect by its behavior. Maybe there is a version field, maybe there isn't. Maybe MySQL 5 and MySQL 6 have different sub-classes.

          Show
          julianhyde Julian Hyde added a comment - I'd rather judge a SqlDialect by its behavior. Maybe there is a version field, maybe there isn't. Maybe MySQL 5 and MySQL 6 have different sub-classes.

            People

            • Assignee:
              jbalint@gmail.com Jess Balint
              Reporter:
              jbalint@gmail.com Jess Balint
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development