[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-887987666 I think we can consider it abandoned. -- 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. To unsubscribe, e-mail: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-667456601 @tianchen92 would you mind starting a thread on the ML, it seems that @jacques-n might not have bandwidth. 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-657172845 > I think 2-4 need more conversation about what we want to expose. I'd definitely avoid introducing a new method (3 on your list) until we figure out what sets of functionality we want. getBuffers() may actually be exactly what we want (with a changed order as necessary). I think it would be valuable to rationalize getFieldBuffers() in this context. @jacques-n My current understanding is there is a cycle here which needs to be broken (@tianchen92 please let me know if understand the issues correcty). 1. IPC VectorUnloader currently relies on getFieldBuffers. It shouldn't. 2. Because of how getFieldBuffers should be used, we shouldn't be setting read/writer indices. But we can't remove the indices setting because it is used in VectorUnloader. 3. we can't use getBuffers in place of getFieldBuffer in VectorUnloader because it does not return buffers in the same order. If this is the case I think introducing a new method and moving away from getBuffers is the least bad option. Silently breaking the contract of getBuffers doesn't seem to be good idea (as witnessed by the length of time this PR has dragged out). IIRC correctly I think the jpython code had to work around getBuffers being inconsistent as well, so a discussion would be good. @jacques-n since you have the most context and historical knowledge would you mind starting a thread on dev@? > Remember that FieldVector and getFieldBuffers() were introduced when we had separate non-nullable and nullable vectors but wanted to treat the non-nullable ones as internal (and thus they didn't expose the FieldVector interface). It seems like we have several operational needs I think this might be have been during a portion of time that I stepped away from the project. 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-656491324 @tianchen92 rereading, after rereading all the comments. I think we should 1. Remove setReaderWriterIndeces in getFieldBuffers 2. Deprecate getBuffers 3. Introduce a new getIpcBuffers which is unambiguously used for writing record batches (i.e. in VectorUnloader). 4. Update documentation where it makes sense based on all this conversation. @jacques-n or someone else from dremio can maybe provide additional insight into how getBuffers is used and whether we really need to keep 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-653722056 @tianchen92 does @jacques-n proposal make sense? 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-653322130 Given how long this PR has been open and approved, I think we should aim to check it in next Tuesday, unless we can come up with a concrete plan by then to help mitigate impact to dremio. CC @jacques-n 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-651509291 Can we leave the old method in place and mark it as deprecated and remove in a later release? 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-643059714 @jacques-n @rymurr do you know the progress of this internal to Dremio? It has been blocked a while on feedback, if we don't here back by Monday, I think we should rebase and merge. 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: us...@infra.apache.org
[GitHub] [arrow] emkornfield commented on pull request #6156: ARROW-7539: [Java] FieldVector getFieldBuffers API should not set reader/writer indices
emkornfield commented on pull request #6156: URL: https://github.com/apache/arrow/pull/6156#issuecomment-629016291 @praveenbingo did you have a chance to investigate? 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: us...@infra.apache.org