Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-1425

Support two-level column structure in INSERT/UPDATE/MERGE

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.10.0
    • Fix Version/s: 1.11.0
    • Component/s: core
    • Labels:

      Issue Links

        Activity

        Hide
        julianhyde Julian Hyde added a comment -

        As part of fixing this bug, please fix the TODO: in MockCatalogReader.

        Show
        julianhyde Julian Hyde added a comment - As part of fixing this bug, please fix the TODO: in MockCatalogReader .
        Hide
        maryannxue Maryann Xue added a comment -

        Yes, sure. We'll no longer need that "simple" table once this issue is fixed. And I just pushed my initial check-in to https://github.com/maryannxue/calcite/tree/phoenix-column-family. All existing test cases and new DML test cases pass with the new approach. Code refinement might be required, but let's start our discussions from there and see if this is the right way to go.

        Show
        maryannxue Maryann Xue added a comment - Yes, sure. We'll no longer need that "simple" table once this issue is fixed. And I just pushed my initial check-in to https://github.com/maryannxue/calcite/tree/phoenix-column-family . All existing test cases and new DML test cases pass with the new approach. Code refinement might be required, but let's start our discussions from there and see if this is the right way to go.
        Hide
        jamestaylor James Taylor added a comment -

        Ping Julian Hyde. Any chance you can review this soon?

        Show
        jamestaylor James Taylor added a comment - Ping Julian Hyde . Any chance you can review this soon?
        Hide
        julianhyde Julian Hyde added a comment -

        The current commit, https://github.com/maryannxue/calcite/commit/3f9b23ae815ca7a77e9b1793e023112c8a387dca, looks good. A couple of comments:

        • Clarify the javadoc for resolveColumn, especially the mechanism whereby unmatched names are removed. The current javadoc is ambiguous due to both passive voice and "should". Write "This method removes ..." or similar in the method description, not the argument javadoc. (I like the mechanism, but mutating arguments is a bit unusual.)
        • Remove the method translateColumnNames. It is not used.
        • The big if..else if... block in MockCatalogReader is unwieldy. Could you replace with a switch? (You can switch on strings since JDK 1.7.) Or could you just concatenate the names using "." and look up the corresponding field? (It's OK, I know you're not relying on dots in field names for real, just in this test.)

        +1 once you've considered these issues.

        Show
        julianhyde Julian Hyde added a comment - The current commit, https://github.com/maryannxue/calcite/commit/3f9b23ae815ca7a77e9b1793e023112c8a387dca , looks good. A couple of comments: Clarify the javadoc for resolveColumn , especially the mechanism whereby unmatched names are removed. The current javadoc is ambiguous due to both passive voice and "should". Write "This method removes ..." or similar in the method description, not the argument javadoc. (I like the mechanism, but mutating arguments is a bit unusual.) Remove the method translateColumnNames . It is not used. The big if..else if... block in MockCatalogReader is unwieldy. Could you replace with a switch? (You can switch on strings since JDK 1.7.) Or could you just concatenate the names using "." and look up the corresponding field? (It's OK, I know you're not relying on dots in field names for real, just in this test.) +1 once you've considered these issues.
        Hide
        maryannxue Maryann Xue added a comment -

        Thank you very much for the review and the comments, Julian Hyde! I was not so sure about mutating arguments either. It's unusual and it might not be very efficient, since the caller always needs to make a copy of the list whereas the callee in most cases either clears the list (if all parts are resolved) or do nothing to the list (if no part can be resolved) and in the former case copy-on-write list won't help. So would it make sense to put it in the return value, for example, like Pair< List<RelDataTypeField>, List<String> > resolveColumn(RelDataTypeFactory typeFactory, List<String>)? or make Pair< List<RelDataTypeField>, List<String> > another class?

        Show
        maryannxue Maryann Xue added a comment - Thank you very much for the review and the comments, Julian Hyde ! I was not so sure about mutating arguments either. It's unusual and it might not be very efficient, since the caller always needs to make a copy of the list whereas the callee in most cases either clears the list (if all parts are resolved) or do nothing to the list (if no part can be resolved) and in the former case copy-on-write list won't help. So would it make sense to put it in the return value, for example, like Pair< List<RelDataTypeField>, List<String> > resolveColumn(RelDataTypeFactory typeFactory, List<String>) ? or make Pair< List<RelDataTypeField>, List<String> > another class?
        Hide
        julianhyde Julian Hyde added a comment -

        I think any API is fine, as long as the javadoc is clear. Probably returning Pair<List<RelDataTypeField>, List<String>> is best.

        Or is it the case that the size of the returned list of fields + the size of the returned list of names is always equal to the initial list of names? If so, you don't need to return a list of remaining names: the caller can always figure out how many were consumed, and how many are left.

        Show
        julianhyde Julian Hyde added a comment - I think any API is fine, as long as the javadoc is clear. Probably returning Pair<List<RelDataTypeField>, List<String>> is best. Or is it the case that the size of the returned list of fields + the size of the returned list of names is always equal to the initial list of names? If so, you don't need to return a list of remaining names: the caller can always figure out how many were consumed, and how many are left.
        Hide
        maryannxue Maryann Xue added a comment -

        No. Every element in List<RelDataTypeField> is parallel to each other. The interface is to process one level of field at a time, but allows multiple matches, which means it does not throw ambiguous-column-exception or column-not-found-exception instantly. It is up to the caller to handle this. For getTargetField() in insert/upsert, the logic is simply throw exception is the resolved field count is not "1" or the left-over names is not empty. For resolveInNamespace(), I think the interface assigns with the logic that the "names" can be matched multiple times and added to "resolved".

        Show
        maryannxue Maryann Xue added a comment - No. Every element in List<RelDataTypeField> is parallel to each other. The interface is to process one level of field at a time, but allows multiple matches, which means it does not throw ambiguous-column-exception or column-not-found-exception instantly. It is up to the caller to handle this. For getTargetField() in insert/upsert, the logic is simply throw exception is the resolved field count is not "1" or the left-over names is not empty. For resolveInNamespace(), I think the interface assigns with the logic that the "names" can be matched multiple times and added to "resolved".
        Hide
        julianhyde Julian Hyde added a comment -

        OK, let's go with Pair<List<RelDataTypeField>, List<String>>.

        Show
        julianhyde Julian Hyde added a comment - OK, let's go with Pair<List<RelDataTypeField>, List<String>> .
        Show
        maryannxue Maryann Xue added a comment - Fixed in https://git1-us-west.apache.org/repos/asf?p=calcite.git;a=commit;h=46654ad .
        Hide
        julianhyde Julian Hyde added a comment -

        Resolved in release 1.11.0 (2017-01-11).

        Show
        julianhyde Julian Hyde added a comment - Resolved in release 1.11.0 (2017-01-11).

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development