Details

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

      Description

      The current Schema#snapshot(long timestamp) interface is limited, for it assumes that users only use timestamp for schema version control, thus we need something new to replace the "timestamp" parameter.
      So we'll introduce a SchemaVersion interface with a partial ordering contract that is:
      1) Irreflexive: !a.isBefore(a), which means a cannot happen before itself;
      2) Transitive: if a.isBefore(b) and b.isBefore(c) then a.isBefore(c);
      and 3) anti-symmetric: if a.isBefore(b) then !b.isBefore(a);
      User can implement their own SchemaVersion, which, aside from following the partial ordering contract, must also override equals(), hashCode() and toString(), so that user can, through overriding CalciteConnection#createPrepareContext(), create a Schema snapshot of a specific SchemaVersion used by the prepare context.

        Issue Links

          Activity

          Hide
          michaelmior Michael Mior added a comment -

          Resolved in release 1.14.0 (2017-10-01)

          Show
          michaelmior Michael Mior added a comment - Resolved in release 1.14.0 (2017-10-01)
          Show
          maryannxue Maryann Xue added a comment - Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=35209136c5b21b83b1d2a3d4180a121f58e3c2f6 .
          Hide
          julianhyde Julian Hyde added a comment -

          The javadoc for interface SchemaVersion could use a little markup. Otherwise those bullet points will flow into a single paragraph.

          With that, +1.

          Show
          julianhyde Julian Hyde added a comment - The javadoc for interface SchemaVersion could use a little markup. Otherwise those bullet points will flow into a single paragraph. With that, +1.
          Hide
          maryannxue Maryann Xue added a comment -

          Committed changes in PR. I removed contentsHaveChangedSince but kept CachingCalciteSchema, which can be useful sometimes (e.g., for AbstractSchema, JsonSchema, etc.). Except that I had to make changes to JdbcTest#testSchemaCaching(), all tests are fine. Let me know if I can go ahead and commit, Julian Hyde.

          Show
          maryannxue Maryann Xue added a comment - Committed changes in PR. I removed contentsHaveChangedSince but kept CachingCalciteSchema , which can be useful sometimes (e.g., for AbstractSchema, JsonSchema, etc.). Except that I had to make changes to JdbcTest#testSchemaCaching() , all tests are fine. Let me know if I can go ahead and commit, Julian Hyde .
          Hide
          maryannxue Maryann Xue added a comment -

          "isBefore" sounds good. I'll change the description accordingly. I'll try removing CachingCalciteSchema and see what happens. Guess it won't break any test other than "caching" specific tests.

          Show
          maryannxue Maryann Xue added a comment - "isBefore" sounds good. I'll change the description accordingly. I'll try removing CachingCalciteSchema and see what happens. Guess it won't break any test other than "caching" specific tests.
          Hide
          julianhyde Julian Hyde added a comment -

          What would be the impact of removing CachingCalciteSchema? It would simplify things, as long as it doesn't break too much.

          OK, it's decided, let's change to strict. Can we call it "isBefore"?

          Show
          julianhyde Julian Hyde added a comment - What would be the impact of removing CachingCalciteSchema? It would simplify things, as long as it doesn't break too much. OK, it's decided, let's change to strict. Can we call it "isBefore"?
          Hide
          maryannxue Maryann Xue added a comment -

          Should we also change Schema.contentsHaveChangedSince(long, long)?

          Actually I had thought about this while I was working on this issue. My conclusion now is that now that we create a new Schema snapshot at the beginning of each query compilation, we should remove this interface at all. Thus the only difference between a CachingCalciteSchema and a SimpleCalciteSchema will be whether the list of schema object names is pre-loaded or not.

          I notice that distributed systems clocks e.g. Lamport timestamps typically use a strict partial order (i.e. !a.happensBefore(a)). Do you think ours be strict also?

          If we remove the above interface, this won't seem to matter to Calcite any more. It's all up to user to figure out what to do in creating a schema snapshot, although I prefer changing the interface to "lessThan" or "before" and requires it to be strict.

          Show
          maryannxue Maryann Xue added a comment - Should we also change Schema.contentsHaveChangedSince(long, long)? Actually I had thought about this while I was working on this issue. My conclusion now is that now that we create a new Schema snapshot at the beginning of each query compilation, we should remove this interface at all. Thus the only difference between a CachingCalciteSchema and a SimpleCalciteSchema will be whether the list of schema object names is pre-loaded or not. I notice that distributed systems clocks e.g. Lamport timestamps typically use a strict partial order (i.e. !a.happensBefore(a)). Do you think ours be strict also? If we remove the above interface, this won't seem to matter to Calcite any more. It's all up to user to figure out what to do in creating a schema snapshot, although I prefer changing the interface to "lessThan" or "before" and requires it to be strict.
          Hide
          maryannxue Maryann Xue added a comment -

          Sorry, Julian Hyde, for my late response on this issue. My hands were tied with some other work. I will get this PR ready ASAP.

          Show
          maryannxue Maryann Xue added a comment - Sorry, Julian Hyde , for my late response on this issue. My hands were tied with some other work. I will get this PR ready ASAP.
          Hide
          julianhyde Julian Hyde added a comment -

          Maryann Xue, I would really like this change to go in before 1.14, so we affect fewer people with the breaking API change.

          This PR is almost ready: the questions above are questions, not objections, so all you MUST do is change the documentation; you could do more, depending on how you answer the questions.

          Show
          julianhyde Julian Hyde added a comment - Maryann Xue , I would really like this change to go in before 1.14, so we affect fewer people with the breaking API change. This PR is almost ready: the questions above are questions, not objections, so all you MUST do is change the documentation; you could do more, depending on how you answer the questions.
          Hide
          julianhyde Julian Hyde added a comment -

          Maryann Xue, Reviewing https://github.com/apache/calcite/pull/515/commits/68cdbab99bcbd719b8159833e4fe16718ec3a101:

          • Can you please add more description of the proposed changes to this JIRA case;
          • Should we also change Schema.contentsHaveChangedSince(long, long)?
          • In SchemaVersion please state the partial order contract more explicitly: reflexive: if a.equals(b) then a.lessThanOrEqualTo(b); transitive: if a. lessThanOrEqualTo(b) and b. lessThanOrEqualTo(c) then a.lessThanOrEqualTo(c); anti-symmetric: if a.lessThanOrEqualTo(b) and !a.equals(b) then !b.lessThanOrEqualTo(a); also, implementations must override equals, hashCode and toString.
          • I notice that distributed systems clocks e.g. Lamport timestamps typically use a strict partial order (i.e. !a.happensBefore(a)). Do you think ours be strict also?
          Show
          julianhyde Julian Hyde added a comment - Maryann Xue , Reviewing https://github.com/apache/calcite/pull/515/commits/68cdbab99bcbd719b8159833e4fe16718ec3a101: Can you please add more description of the proposed changes to this JIRA case; Should we also change Schema.contentsHaveChangedSince(long, long) ? In SchemaVersion please state the partial order contract more explicitly: reflexive: if a.equals(b) then a.lessThanOrEqualTo(b); transitive: if a. lessThanOrEqualTo(b) and b. lessThanOrEqualTo(c) then a.lessThanOrEqualTo(c); anti-symmetric: if a.lessThanOrEqualTo(b) and !a.equals(b) then !b.lessThanOrEqualTo(a); also, implementations must override equals, hashCode and toString. I notice that distributed systems clocks e.g. Lamport timestamps typically use a strict partial order (i.e. !a.happensBefore(a) ). Do you think ours be strict also?
          Hide
          maryannxue Maryann Xue added a comment -
          Show
          maryannxue Maryann Xue added a comment - Julian Hyde , could you please review https://github.com/apache/calcite/pull/515 for me?

            People

            • Assignee:
              maryannxue Maryann Xue
              Reporter:
              maryannxue Maryann Xue
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development