-
Notifications
You must be signed in to change notification settings - Fork 245
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Enable Strings as a supported type for GpuColumnarToRow transitions #5998
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me> fix the type of the dataOffsetTmp Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits. It is looking good
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuRowToColumnarExec.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Nice to see that it isn't a massive change, but it is dense in parts. Sorry for being so pedantic, I was just writing things down as I went and looking back at the number of comments this appears that I have huge requests for this code, but I really don't feel that way. Thanks for doing this work!
|
||
/** | ||
* Calculates the offset of the variable width section. | ||
* @return Total bytes used by the fixed width and the validity bytes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A note about alignment here would be nice. In this case, we are byte aligned at the end of the validity data and we require no alignment, but calling that out would be useful for when we forget and get worried about alignment and look it up in the format spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
@@ -111,9 +102,10 @@ class AcceleratedColumnarToRowIterator( | |||
// most 184 double/long values. Spark by default limits codegen to 100 fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments still correct?
val cudfColOff = jcudfRowVisitor.getColOffset() | ||
val colLength = jcudfRowVisitor.getColLength() | ||
val ret = colLength match { | ||
// strings return -15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very magical. Why -15? Should this be some sort of define?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored. Not needed any more. Util
will return the generated-code.
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
|
||
/** | ||
* A helper class to calculate the columns, validity, and variable width offsets. | ||
* This helper is used to get an estimate size for the row including variable-sized rows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* This helper is used to get an estimate size for the row including variable-sized rows. | |
* This helper is used to get an estimate size for the row including variable-sized data. |
for (int i = 0; i < attributes.length; i++) { | ||
calcColOffset(i); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to throw away the offsets you're calculating from the return value, but now I see the goal is the side-effect in the function of setting varSizeColIndex
. I wouldn't have thought much of this had this called setCOlumnOffsets
and I would have missed the side-effect entirely. I wonder if something should change around here to better name, indicate the side-effect, or not require offset calculation at all if it has been done before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done! changed the name to advanceColCursorAndGet
return byteCursor; | ||
} | ||
private int addVarSizeData(DType rapidsType) { | ||
int length = getEstimateSizeForVarLengthTypes(rapidsType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if this estimate is low? Is this used for buffer allocation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handled as described in #5998 (comment)
* @param ind index of the column | ||
* @return the offset of the colum. | ||
*/ | ||
private int calcColOffset(int ind) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very familiar. Would it make sense to wrap the above class in a caching class or is it different enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was avoiding inheritance to reduce the overhead. That's why I opted to repeat the code rather than using overrides/interfaces.
In the most recent code commit, the two inner classes have different functionality.
JCudfUtil.RowOffsetsCalculator
: is used to calculate the offsets and estimate size for the buffer allocation.JCudfUtil.RowBuilder
: it iterates on the columns to generate the code used to copy the column field.
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
build |
I see the following failures in the Pre-merge:
|
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
build |
, " long cudfDstAddress = startAddress + dataDstOffset;" | ||
, " long newDataOffset = cudfDstAddress + strSize;" | ||
, " if (newDataOffset > bufferEndAddress) {" | ||
, " throw new java.lang.RuntimeException(" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is now any time we try to copy a String that does not fit, this will throw an exception. I am not okay with that because there can be many cases where we want to copy more than one batch to the GPU. I am fine with throwing an exception if the first batch cannot fit, but we do need a way to detect that later rows don't fit and just go on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Bobby!
I modified the behavior to handle the corner case.
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
Reasons of old failure in
The fix: The following changes fixes the unit tests.
batchDone = false;
int retryCount = 0;
while (!batchDone) {
try (HostMemoryBuffer dataBuffer = HostMemoryBuffer.allocate(dataLength);
HostMemoryBuffer offsetsBuffer =
HostMemoryBuffer.allocate(((long)numRowsEstimate + 1) * BYTES_PER_OFFSET)) {
int[] used = fillBatch(dataBuffer, offsetsBuffer);
int dataOffset = used[0];
int currentRow = used[1];
// if we fail to copy at least one row then we need to throw an exception
if (currentRow == 0 && pending != null) {
throw new BufferOverflowException();
}
batchDone = true
if (retryCount > 0) {
// restore the original dataLength and numRowsEstimate
// so that we do not continue using outliers.
}
// rest code to create and copy CV here
....
...
} catch (BufferOverflowException ex) { // batch does not fit a single row
// increase dataLength by 25%
int newDataLength = Math.Min(Integer.MAX_VALUE, (dataLength * 125) / 100));
if (newDataLength <= dataLength) { // we already reached the limit
throw RuntimeException(ex);
}
dataLength = newDataLength;
numRowsEstimate = //calculate new value;
retryCount++;
}
} @revans2 and @hyperbolic2346 let me know if you are fine with the new fix. |
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
build |
Why wouldn't we start with a target size of a single page of memory? Do we really need to get it down to the byte if we increase it by 25% each time?
Is this a valid assumption for strings? cudf limits a single column to 2 gigs, but that could be a single row. Further, there could be any number of string columns in a table. This limits a single row to ~50% of the gpu memory available before we are unable to convert it due to the double allocation, etc. If we assume 20 bytes per string and the string is even remotely large, adding 25% each iteration will take a long time to get up to large enough.
Can the failure case return the number of bytes required so we can better tune the allocation?
Would this process have to repeat for each string then? Say we have a row with 10 strings that are each 1 meg. Would we have to call into it 10 times in the best case where we fail once and are able to resize to a large enough buffer to hold the failed allocation? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there for sure. These are just nits and questions.
long newRowSizeEst = dataLength << 1 ; | ||
newRowSizeEst = Math.min(newRowSizeEst, JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long newRowSizeEst = dataLength << 1 ; | |
newRowSizeEst = Math.min(newRowSizeEst, JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH); | |
long newRowSizeEst = Math.min(dataLength << 1, JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH); |
It looks odd to me to write to newRowSizeEst and then immediately assign to it again. This got me looking more at this code and now I have questions too.
So we have a buffer of size dataLength and it is too small, so we double the buffer size in newRowSizeEst and then set it to the min of that doubled buffer size and a max. Then we compare the new size we want to make the buffer to the old buffer size and if it is smaller or the same, we bail. The only time this could happen is if dataLength is currently JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH
, right? If that is the case, why won't we just check that? This way seems a little convoluted.
if (dataLength == JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH) { // this won't work...
// double buffer size and try again
long newRowSizeEst = Math.min(dataLength << 1, JCudfUtil.JCUDF_MAX_DATA_BUFFER_LENGTH);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sure. Easy fix.
@@ -62,7 +62,8 @@ public final class JCudfUtil { | |||
/** | |||
* The maximum buffer size allocated to copy JCudf row. | |||
*/ | |||
public static final long JCUDF_MAX_DATA_BUFFER_LENGTH = Integer.MAX_VALUE; | |||
public static final long JCUDF_MAX_DATA_BUFFER_LENGTH = | |||
Integer.MAX_VALUE - (JCUDF_ROW_ALIGNMENT - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using Integet.MAX_VALUE
as a limit? It is a limit on a cudf column due to cudf::size_type
being a signed int, but is there a reason to limit the row here to an int? I'm not against this, just ensuring it isn't being coupled unnecessarily to cudf::size_type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During transitions, the rows are 8-bytes aligned. So, Integer.MAX_VALUE - 7
is 8 bytes aligned.
Probably I should rename this to a JCUDF_MAX_ROW_SIZE_LENGTH
because the bufferSize which is multiple rows can be larger.
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
build |
Failure in CI is unrelated and being reported in #6054 |
build |
1 similar comment
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Most of my comments are about comments, so I think we can declare this very close.
sql-plugin/src/main/java/com/nvidia/spark/rapids/CudfUnsafeRow.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/CudfUnsafeRow.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/InternalRowToColumnarBatchIterator.java
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
sql-plugin/src/main/java/com/nvidia/spark/rapids/JCudfUtil.java
Outdated
Show resolved
Hide resolved
return attributes.length > 0 && varSizeColIndex != attributes.length; | ||
} | ||
|
||
public int getValidityBytesOffset() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing some new functions without comments. I'm not sure the comment on function policy of the java code though, so I'm just going to comment that I see it. :)
….java Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
….java Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Co-authored-by: Mike Wilson <hyperbolic2346@users.noreply.github.com>
Signed-off-by: Ahmed Hussein (amahussein) <a@ahussein.me>
buil |
build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiniest of nits left, I'm happy with how this has turned out. Thank you for humoring me through so many tiny changes.
…itions (NVIDIA#5998)" This reverts commit 452e7ba. Signed-off-by: Jason Lowe <jlowe@nvidia.com>
Signed-off-by: Ahmed Hussein (amahussein) a@ahussein.me
fixes #5633, fixes #5634, fixes #5635, fixes #5636
Only fixed width schema were supported by the
GpuColumnarToRow
transitions.This PR adds stringType to the list of supported columns.
Changes:
Addressing points in rapidsai/cudf#10033 (comment)
AcceleratedColumnarToRowIterator
and how the packMap is constructed.row_conversion_test.py