Skip to content

[CALCITE-2209] Support loading model file through URL#701

Closed
walterddr wants to merge 4 commits intoapache:masterfrom
walterddr:CALCITE-2209
Closed

[CALCITE-2209] Support loading model file through URL#701
walterddr wants to merge 4 commits intoapache:masterfrom
walterddr:CALCITE-2209

Conversation

@walterddr
Copy link

@walterddr walterddr commented May 25, 2018

@suez1224
Copy link

suez1224 commented Jun 5, 2018

Can we fix the test errors?

@walterddr
Copy link
Author

@suez1224 I pushed a fix on all the test errors regarding the upgrade from File based to URI based in all SchemaFactory/TableFactory. I am not sure whether this is categorized as backward incompatible change or not. Please take a look.

@walterddr walterddr changed the title [CALCITE-2209] adding in JSON URL support for ModelHandler [CALCITE-2209] Support loading model file through URL Nov 25, 2018
@AsmaZgo
Copy link

AsmaZgo commented Dec 18, 2018

thank you for addressing this feature! it will improve the quality of my tool.

String fileName = (String) operand.get("file");
final File base =
(File) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
final URI uri = (URI) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
Copy link
Contributor

Choose a reason for hiding this comment

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

@walterddr , this looks like a backward-incompatible change. In other words, clients have to update their code even though they don't use URIs yet.

Have you though of a possibility to keep previous code intact while allowing to leverage new API if required?

Copy link
Author

Choose a reason for hiding this comment

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

hmm. this was confusing to me. I was not sure about the original implementation to directly convert the Object to File.

How about:

Object baseDirObj = operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
Final File base;
if (baseDirObj instanceof URI) {
  uri = (URI) baseDirObj;
  base = new File(uri);
} else if (baseDirObj instanceof File) {
  base = (File) baseDirObj;
} else {
  throw IllegalArgumentException(...);
}

Copy link
Author

@walterddr walterddr left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback @vlsi . sorry for the late reply, I just replied with a possible solution, please kindly take a look and see if I am heading the right direction.

String fileName = (String) operand.get("file");
final File base =
(File) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
final URI uri = (URI) operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
Copy link
Author

Choose a reason for hiding this comment

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

hmm. this was confusing to me. I was not sure about the original implementation to directly convert the Object to File.

How about:

Object baseDirObj = operand.get(ModelHandler.ExtraOperand.BASE_DIRECTORY.camelName);
Final File base;
if (baseDirObj instanceof URI) {
  uri = (URI) baseDirObj;
  base = new File(uri);
} else if (baseDirObj instanceof File) {
  base = (File) baseDirObj;
} else {
  throw IllegalArgumentException(...);
}

break;
case BASE_DIRECTORY:
File f = null;
URI f;
Copy link
Author

Choose a reason for hiding this comment

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

@vlsi one more comment on this is that I changed this to always use URI as Object type in the operandMap. I am assuming this is also a backward-incompatible change since other users who directly alter the operandMap will also have to change File type to URI type.

Any suggestions on how to make it backward compatible. My initial thought would be to give another key, say:

case BASE_DIRECTORY:
  // ...
case BASE_DIRECTORY_URI:
  // ... 

and mark BASE_DIRECTORY as deprecated key

@nemccarthy
Copy link

Shame this didnt make it in. We could really use this feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants