[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[shivzone]

Github user shivzone closed the pull request at:

https://github.com/apache/incubator-hawq/pull/1332


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[denalex]

Github user denalex commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163696192
  
--- Diff: 
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/utilities/ProtocolData.java
 ---
@@ -82,19 +83,26 @@ public ProtocolData(Map paramsMap) {
 parseTupleDescription();
 
 /*
- * accessor - will throw exception from getPropery() if 
outputFormat is
+ * accessor - will throw exception if outputFormat is
  * BINARY and the user did not supply accessor=... or profile=...
- * resolver - will throw exception from getPropery() if 
outputFormat is
+ * resolver - will throw exception if outputFormat is
  * BINARY and the user did not supply resolver=... or profile=...
  */
-profile = getOptionalProperty("PROFILE");
+profile = getUserProperty("PROFILE");
 if (profile != null) {
 setProfilePlugins();
 }
-accessor = getProperty("ACCESSOR");
-resolver = getProperty("RESOLVER");
-fragmenter = getOptionalProperty("FRAGMENTER");
-metadata = getOptionalProperty("METADATA");
+accessor = getUserProperty("ACCESSOR");
+if(accessor == null) {
+protocolViolation(accessor);
--- End diff --

what's the point of sending null to protocolViolation function ? would you 
want to send string instead ?


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[denalex]

Github user denalex commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163695957
  
--- Diff: 
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/utilities/ProtocolData.java
 ---
@@ -82,19 +83,26 @@ public ProtocolData(Map paramsMap) {
 parseTupleDescription();
 
 /*
- * accessor - will throw exception from getPropery() if 
outputFormat is
+ * accessor - will throw exception if outputFormat is
  * BINARY and the user did not supply accessor=... or profile=...
- * resolver - will throw exception from getPropery() if 
outputFormat is
+ * resolver - will throw exception if outputFormat is
  * BINARY and the user did not supply resolver=... or profile=...
  */
-profile = getOptionalProperty("PROFILE");
+profile = getUserProperty("PROFILE");
--- End diff --

but you repeat the same code multiple times here -- throwing error if the 
property is not present, which makes it a mandatory "user property".


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[shivzone]

Github user shivzone commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163680129
  
--- Diff: 
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/utilities/ProtocolData.java
 ---
@@ -82,19 +83,26 @@ public ProtocolData(Map paramsMap) {
 parseTupleDescription();
 
 /*
- * accessor - will throw exception from getPropery() if 
outputFormat is
+ * accessor - will throw exception if outputFormat is
  * BINARY and the user did not supply accessor=... or profile=...
- * resolver - will throw exception from getPropery() if 
outputFormat is
+ * resolver - will throw exception if outputFormat is
  * BINARY and the user did not supply resolver=... or profile=...
  */
-profile = getOptionalProperty("PROFILE");
+profile = getUserProperty("PROFILE");
--- End diff --

I used getUserProperty since I thought User property implicitly indicates 
that these are custom properties


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[shivzone]

Github user shivzone commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163679669
  
--- Diff: 
pxf/pxf-jdbc/src/main/java/org/apache/hawq/pxf/plugins/jdbc/JdbcPlugin.java ---
@@ -58,8 +58,8 @@ public JdbcPlugin(InputData input) throws 
UserDataException {
 super(input);
 jdbcDriver = input.getUserProperty("JDBC_DRIVER");
 dbUrl = input.getUserProperty("DB_URL");
-user = input.getUserProperty("USER");
-pass = input.getUserProperty("PASS");
+user = input.getUserProperty("DB_USER");
+pass = input.getUserProperty("DB_PASS");
--- End diff --

yes ... we don't need this


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[denalex]

Github user denalex commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163433718
  
--- Diff: 
pxf/pxf-jdbc/src/main/java/org/apache/hawq/pxf/plugins/jdbc/JdbcPlugin.java ---
@@ -58,8 +58,8 @@ public JdbcPlugin(InputData input) throws 
UserDataException {
 super(input);
 jdbcDriver = input.getUserProperty("JDBC_DRIVER");
 dbUrl = input.getUserProperty("DB_URL");
-user = input.getUserProperty("USER");
-pass = input.getUserProperty("PASS");
+user = input.getUserProperty("DB_USER");
+pass = input.getUserProperty("DB_PASS");
--- End diff --

no longer necessary to change these property names


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[denalex]

Github user denalex commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163433442
  
--- Diff: 
pxf/pxf-api/src/test/java/org/apache/hawq/pxf/api/utilities/ProfilesConfTest.java
 ---
@@ -96,16 +96,18 @@ public void definedProfile() throws Exception {
 optionalFile.toURI().toURL());
 
 Map hbaseProfile = 
ProfilesConf.getProfilePluginsMap("HBase");
+System.out.println(hbaseProfile);
+System.out.println(hbaseProfile.get("X-GP-OPTIONS-PLUGIN1"));
--- End diff --

did you intend to have System.out here ?


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[denalex]

Github user denalex commented on a diff in the pull request:

https://github.com/apache/incubator-hawq/pull/1332#discussion_r163434200
  
--- Diff: 
pxf/pxf-service/src/main/java/org/apache/hawq/pxf/service/utilities/ProtocolData.java
 ---
@@ -82,19 +83,26 @@ public ProtocolData(Map paramsMap) {
 parseTupleDescription();
 
 /*
- * accessor - will throw exception from getPropery() if 
outputFormat is
+ * accessor - will throw exception if outputFormat is
  * BINARY and the user did not supply accessor=... or profile=...
- * resolver - will throw exception from getPropery() if 
outputFormat is
+ * resolver - will throw exception if outputFormat is
  * BINARY and the user did not supply resolver=... or profile=...
  */
-profile = getOptionalProperty("PROFILE");
+profile = getUserProperty("PROFILE");
--- End diff --

should we preserve semantic of getUserProperty() that throws exception if 
the property is not present and getUserOptionalProperty() that returns null ?


---

[GitHub] incubator-hawq pull request #1332: HAWQ-1581. Separate PXF system parameters...

[shivzone]

GitHub user shivzone opened a pull request:

https://github.com/apache/incubator-hawq/pull/1332

HAWQ-1581. Separate PXF system parameters from user parameters



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/shivzone/incubator-hawq HAWQ-1581

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/incubator-hawq/pull/1332.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #1332


commit f949858090dd5c7848df79434be5301f3c193489
Author: shivzone 
Date:   2018-01-24T00:29:41Z

HAWQ-1581. Separate PXF system parameters from user configurable visible 
parameters




---