You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/05/27 03:53:38 UTC
[GitHub] [arrow] nealrichardson opened a new pull request #7281: WIP Put more things in type_fwds
nealrichardson opened a new pull request #7281:
URL: https://github.com/apache/arrow/pull/7281
cc @fsaintjacques @bkietz
This goes on the assumptions that (1) all arrowExports.cpp needs are `**/type_fwd.h` and (2) that doing so would speed up compile time. My understanding is that (1) is true, assuming the type_fwd headers are complete enough, but IDK how much of an effect changing this has on (2).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#discussion_r437017534
##########
File path: python/pyarrow/includes/libarrow.pxd
##########
@@ -1186,10 +1186,10 @@ cdef extern from "arrow/io/api.h" namespace "arrow::io" nogil:
cdef extern from "arrow/ipc/api.h" namespace "arrow::ipc" nogil:
- enum MessageType" arrow::ipc::Message::Type":
- MessageType_SCHEMA" arrow::ipc::Message::SCHEMA"
- MessageType_RECORD_BATCH" arrow::ipc::Message::RECORD_BATCH"
- MessageType_DICTIONARY_BATCH" arrow::ipc::Message::DICTIONARY_BATCH"
+ enum MessageType" arrow::ipc::MessageType":
+ MessageType_SCHEMA" arrow::ipc::MessageType::SCHEMA"
+ MessageType_RECORD_BATCH" arrow::ipc::MessageType::RECORD_BATCH"
+ MessageType_DICTIONARY_BATCH" arrow::ipc::MessageType::DICTIONARY_BATCH"
Review comment:
@bkietz my guess is that this is the line that is causing the lint error, IIUC
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-640902096
@bkietz would you mind finishing this up and merging? Looks like there's Windows C++ test failures and an odd lint failure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm edited a comment on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-641545625
appveyor is broken on master, not sure if this patch caused it. I compiled on Windows locally and couldn't reproduce so may be transient
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-641602964
Maybe so; odd that Appveyor appears not to have run on the last commit here. It looks like the failures are compression related, and Ben's last commit seems to be trying to address that area.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7281: WIP Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-636918597
I can help with this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-641630643
I opened https://issues.apache.org/jira/browse/ARROW-9085
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm edited a comment on pull request #7281: WIP Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-636918597
I can help with this if needed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-641545625
appveyor is broken on master, not sure if this patch caused it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-639827383
@github-actions rebase
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7281: WIP Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-636042815
Alright, I'll tinker a bit more with this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz closed pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
bkietz closed pull request #7281:
URL: https://github.com/apache/arrow/pull/7281
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7281: WIP Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-634297613
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW
Then could you also rename pull request title in the following format?
ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7281: ARROW-9046: [C++][R] Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-641010768
https://issues.apache.org/jira/browse/ARROW-9046
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #7281: WIP Put more things in type_fwds
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #7281:
URL: https://github.com/apache/arrow/pull/7281#issuecomment-636086599
I did a little bit more--and judging from the size of the arrowExports.o file, it had an effect--but could use some help going beyond this. I don't know how to forward declare things like `arrow::ipc::Message::Type` since Message is a class and not a namespace, for example.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org