You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hawq.apache.org by kavinderd <gi...@git.apache.org> on 2016/07/14 23:16:10 UTC
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
GitHub user kavinderd opened a pull request:
https://github.com/apache/incubator-hawq/pull/796
HAWQ-927. Pass ProjectionInfo data to PXF
This commit makes the necessary modifications to the HAWQ side of
the codebase to add a list of indices of projected columns
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kavinderd/incubator-hawq HAWQ-927
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-hawq/pull/796.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 #796
----
commit 68c6dd7ea6487b8362627fd0a4aa8c25a355abd3
Author: Kavinder Dhaliwal <ka...@gmail.com>
Date: 2016-07-12T01:20:16Z
HAWQ-927. Pass ProjectionInfo data to PXF
This commit makes the necessary modifications to the HAWQ side of
the codebase to add a list of indices of projected columns
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71558813
--- Diff: src/backend/access/external/fileam.c ---
@@ -454,6 +454,21 @@ external_stopscan(FileScanDesc scan)
}
}
+/* ----------------
+ * external_getnext_init - prepare ExternalSelectDesc struct before external_getnext
+/* ----------------
+ */
+
+ExternalSelectDesc
+external_getnext_init(PlanState *state) {
+ ExternalSelectDesc desc = (ExternalSelectDesc) palloc0(sizeof(ExternalSelectDescData));
--- End diff --
I missed adding `pfree` for `desc`. I added it to the end of `ExternalNext()`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71023742
--- Diff: src/include/access/fileam.h ---
@@ -63,6 +63,17 @@ typedef struct ExternalInsertDescData
typedef ExternalInsertDescData *ExternalInsertDesc;
+/*
+ * ExternalSelectDescData is used for storing state related
+ * to selecting data from an external table.
+ */
+typedef struct ExternalSelectDescData
--- End diff --
Makes sense
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq issue #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on the issue:
https://github.com/apache/incubator-hawq/pull/796
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r70901193
--- Diff: src/include/access/fileam.h ---
@@ -63,6 +63,17 @@ typedef struct ExternalInsertDescData
typedef ExternalInsertDescData *ExternalInsertDesc;
+/*
+ * ExternalSelectDescData is used for storing state related
+ * to selecting data from an external table.
+ */
+typedef struct ExternalSelectDescData
--- End diff --
Why do we introduce new struct if we can just use ProjectionInfo?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-hawq/pull/796
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71559534
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +166,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[sizeof(int32) * 8];
+ int *varNumbers = projInfo->pi_varNumbers;
+ StringInfoData formatter;
+ initStringInfo(&formatter);
+
+ /* Convert the number of projection columns to a string */
+ pg_ltoa(list_length(projInfo->pi_targetlist), long_number);
+ churl_headers_append(headers, "X-GP-ATTRS-PROJ", long_number);
+
+ for(i = 0; i < list_length(projInfo->pi_targetlist); i++) {
--- End diff --
Yes it will be in another PR related to [this](https://issues.apache.org/jira/browse/HAWQ-583?jql=project%20%3D%20HAWQ%20AND%20resolution%20%3D%20Unresolved%20AND%20assignee%20%3D%20kavinderd%20ORDER%20BY%20priority%20DESC) Jira.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71022820
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +165,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[32];
--- End diff --
You can try something like:
char long_number[(sizeof int) * 8];
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71456986
--- Diff: src/backend/access/external/fileam.c ---
@@ -462,7 +462,7 @@ external_stopscan(FileScanDesc scan)
ExternalSelectDesc
external_getnext_init(PlanState *state) {
ExternalSelectDesc desc = (ExternalSelectDesc) palloc0(sizeof(ExternalSelectDescData));
- if (state != NULL && state->ps_ProjInfo != NULL) {
+ if (state != NULL) {
desc->projInfo = state->ps_ProjInfo;
} else {
--- End diff --
You should also delete this `else-branch`, it is unnecessary too.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71460032
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -123,7 +130,8 @@ static void add_alignment_size_httpheader(CHURL_HEADERS headers)
*/
static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
{
- char long_number[32];
+ char long_number[sizeof(int32) * 8];
--- End diff --
I like this modification!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71460581
--- Diff: src/backend/access/external/fileam.c ---
@@ -454,6 +454,21 @@ external_stopscan(FileScanDesc scan)
}
}
+/* ----------------
+ * external_getnext_init - prepare ExternalSelectDesc struct before external_getnext
+/* ----------------
--- End diff --
remove `/` here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r70905516
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +165,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[32];
--- End diff --
Since we are converting `list_length(projInfo->pi_targetlist)`, we need an array thats 32 chars long to match the bit length of `int`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71460571
--- Diff: src/backend/access/external/fileam.c ---
@@ -454,6 +454,21 @@ external_stopscan(FileScanDesc scan)
}
}
+/* ----------------
+ * external_getnext_init - prepare ExternalSelectDesc struct before external_getnext
+/* ----------------
+ */
+
+ExternalSelectDesc
+external_getnext_init(PlanState *state) {
+ ExternalSelectDesc desc = (ExternalSelectDesc) palloc0(sizeof(ExternalSelectDescData));
--- End diff --
I just want to make sure when does `desc` object be freed?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71024372
--- Diff: src/include/access/url.h ---
@@ -154,12 +154,15 @@ typedef struct extvar_t
char GP_CSVOPT[13]; /* "m.x...q...h." former -q, -h and -x options for gpfdist.*/
} extvar_t;
+//struct ExternalSelectDescData;
--- End diff --
Cleanup comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r70904822
--- Diff: src/include/access/fileam.h ---
@@ -63,6 +63,17 @@ typedef struct ExternalInsertDescData
typedef ExternalInsertDescData *ExternalInsertDesc;
+/*
+ * ExternalSelectDescData is used for storing state related
+ * to selecting data from an external table.
+ */
+typedef struct ExternalSelectDescData
--- End diff --
That's an option but since we plan on passing other data like the type of query, etc to PXF I think it will be more maintainable to have this encapsulating struct so that we don't need to keep changing the method signatures.
What are your thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by sansanichfb <gi...@git.apache.org>.
Github user sansanichfb commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r70901067
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +165,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[32];
--- End diff --
Why 32?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq issue #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on the issue:
https://github.com/apache/incubator-hawq/pull/796
LGTM. Remember rebasing the commit before checking in. \U0001f37a
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71459314
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +166,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[sizeof(int32) * 8];
+ int *varNumbers = projInfo->pi_varNumbers;
+ StringInfoData formatter;
+ initStringInfo(&formatter);
+
+ /* Convert the number of projection columns to a string */
+ pg_ltoa(list_length(projInfo->pi_targetlist), long_number);
+ churl_headers_append(headers, "X-GP-ATTRS-PROJ", long_number);
--- End diff --
indent~
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq issue #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on the issue:
https://github.com/apache/incubator-hawq/pull/796
I don't think u need more than 10 bytes for long_number to store a long int.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by shivzone <gi...@git.apache.org>.
Github user shivzone commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71393037
--- Diff: src/backend/access/external/fileam.c ---
@@ -454,6 +454,21 @@ external_stopscan(FileScanDesc scan)
}
}
+/* ----------------
+ * external_getnext_init - prepare ExternalSelectDesc struct before external_getnext
+/* ----------------
+ */
+
+ExternalSelectDesc
+external_getnext_init(PlanState *state) {
+ ExternalSelectDesc desc = (ExternalSelectDesc) palloc0(sizeof(ExternalSelectDescData));
+ if (state != NULL && state->ps_ProjInfo != NULL) {
+ desc->projInfo = state->ps_ProjInfo;
--- End diff --
Second check is redundant
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq issue #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by kavinderd <gi...@git.apache.org>.
Github user kavinderd commented on the issue:
https://github.com/apache/incubator-hawq/pull/796
Please review @xunzhang @paul-guo- @huor
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-hawq pull request #796: HAWQ-927. Pass ProjectionInfo data to PXF
Posted by xunzhang <gi...@git.apache.org>.
Github user xunzhang commented on a diff in the pull request:
https://github.com/apache/incubator-hawq/pull/796#discussion_r71461014
--- Diff: src/backend/access/external/pxfheaders.c ---
@@ -158,6 +166,29 @@ static void add_tuple_desc_httpheader(CHURL_HEADERS headers, Relation rel)
pfree(formatter.data);
}
+static void add_projection_desc_httpheader(CHURL_HEADERS headers, ProjectionInfo *projInfo) {
+ int i;
+ char long_number[sizeof(int32) * 8];
+ int *varNumbers = projInfo->pi_varNumbers;
+ StringInfoData formatter;
+ initStringInfo(&formatter);
+
+ /* Convert the number of projection columns to a string */
+ pg_ltoa(list_length(projInfo->pi_targetlist), long_number);
+ churl_headers_append(headers, "X-GP-ATTRS-PROJ", long_number);
+
+ for(i = 0; i < list_length(projInfo->pi_targetlist); i++) {
--- End diff --
@kavinderd What' the code logic of PXF end to parse this header? Does it be included in another pull request?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---