Skip to content

Encapsulating currentSize in ValidityBuffer #14665

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
JaroslavTulach wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Encapsulating currentSize in ValidityBuffer #14665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 0 additions & 17 deletions ...ts/table/src/main/java/org/enso/table/data/column/builder/BoundCheckedIntegerBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
package org.enso.table.data.column.builder;

import org.enso.base.polyglot.NumericConverter;
import org.enso.table.data.column.operation.cast.CastProblemAggregator;
import org.enso.table.data.column.storage.type.IntegerType;
import org.enso.table.error.ValueTypeMismatchException;
import org.enso.table.problems.ProblemAggregator;

/** A LongBuilder that ensures values it is given fit the target type. */
Expand Down Expand Up @@ -34,21 +32,6 @@ public BoundCheckedIntegerBuilder appendLong(long value) {
return this;
}

@Override
public BoundCheckedIntegerBuilder append(Object o) {
if (o == null) {
appendNulls(1);
} else {
Long x = NumericConverter.tryConvertingToLong(o);
if (x != null) {
appendLong(x);
} else {
throw new ValueTypeMismatchException(type, o);
}
}
return this;
}

@Override
public IntegerType getType() {
return type;
Expand Down
44 changes: 15 additions & 29 deletions std-bits/table/src/main/java/org/enso/table/data/column/builder/DateBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@
import org.enso.table.data.column.storage.type.DateTimeType;
import org.enso.table.data.column.storage.type.DateType;
import org.enso.table.data.column.storage.type.StorageType;
import org.enso.table.error.ValueTypeMismatchException;

/** A builder for LocalDate columns. */
final class DateBuilder extends ValidityBuilder
final class DateBuilder extends ValidityBuilder<DateBuilder>
implements BuilderForType<LocalDate>, BuilderWithRetyping {
private final boolean allowDateToDateTimeConversion;
private IntBuffer data;
Expand Down Expand Up @@ -54,26 +53,9 @@ public boolean accepts(Object o) {
}

@Override
public DateBuilder append(Object o) {
ensureSpaceToAppend();
if (o == null) {
appendNulls(1);
} else {
try {
var local = (LocalDate) o;
this.setValid(currentSize);
data.put(currentSize++, Math.toIntExact(local.toEpochDay()));
} catch (ClassCastException e) {
throw new ValueTypeMismatchException(getType(), o);
}
}
return this;
}

@Override
public DateBuilder appendNulls(int count) {
doAppendNulls(count);
return this;
protected final void appendAt(int at, Object o) throws ClassCastException {
var local = (LocalDate) o;
data.put(at, Math.toIntExact(local.toEpochDay()));
}

@Override
Expand All @@ -86,15 +68,15 @@ public void appendBulkStorage(ColumnStorage<?> storage) {
}

@Override
public boolean canRetypeTo(StorageType<?> type) {
public final boolean canRetypeTo(StorageType<?> type) {
return allowDateToDateTimeConversion && Objects.equals(type, DateTimeType.INSTANCE);
}

@Override
public Builder retypeTo(StorageType<?> type) {
if (allowDateToDateTimeConversion && Objects.equals(type, DateTimeType.INSTANCE)) {
public final Builder retypeTo(StorageType<?> type) {
if (canRetypeTo(type)) {
var res = new DateTimeBuilder(data.capacity(), true);
for (int i = 0; i < currentSize; i++) {
for (int i = 0; i < getCurrentSize(); i++) {
res.append(getData(i));
}
return res;
Expand All @@ -111,7 +93,7 @@ protected int getDataSize() {
@Override
protected void resize(int desiredCapacity) {
var newData = allocBuffer(desiredCapacity, 0);
int toCopy = Math.min(currentSize, data.capacity());
int toCopy = Math.min(currentSize(), data.capacity());
newData.put(0, this.data, 0, toCopy);
this.data = newData;
}
Expand All @@ -123,7 +105,7 @@ public ColumnStorage<LocalDate> seal() {

final Storage<LocalDate> seal(ColumnStorage<?> otherStorage) {
ensureFreeSpaceFor(0);
var buf = data.asReadOnlyBuffer().position(0).limit(currentSize);
var buf = data.asReadOnlyBuffer().position(0).limit(currentSize());
var validity = this.validityMap();

return new DateStorage(buf, validity, otherStorage);
Expand All @@ -136,12 +118,16 @@ public StorageType<LocalDate> getType() {

@Override
public void copyDataTo(Object[] items) {
for (var i = 0; i < items.length && i < currentSize; i++) {
for (var i = 0; i < items.length && i < currentSize(); i++) {
items[i] = getData(i);
}
}

private final LocalDate getData(int i) {
return LocalDate.ofEpochDay(data.get(i));
}

private final int currentSize() {
return Math.toIntExact(getCurrentSize());
}
}
34 changes: 11 additions & 23 deletions std-bits/table/src/main/java/org/enso/table/data/column/builder/DoubleBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import org.enso.table.problems.ProblemAggregator;

/** A builder for floating point columns. */
sealed class DoubleBuilder extends ValidityBuilder implements BuilderForDouble
sealed class DoubleBuilder extends ValidityBuilder<DoubleBuilder> implements BuilderForDouble
permits InferredDoubleBuilder {
protected final PrecisionLossAggregator precisionLossAggregator;
private DoubleBuffer data;
Expand Down Expand Up @@ -79,7 +79,7 @@ final double getData(int i) {
@Override
protected void resize(int desiredCapacity) {
var newData = allocBuffer(desiredCapacity, 0);
int toCopy = Math.min(currentSize, data.capacity());
int toCopy = Math.min(currentSize(), data.capacity());
newData.put(0, data, 0, toCopy);
data = newData;
}
Expand All @@ -99,17 +99,7 @@ public StorageType<Double> getType() {
}

@Override
public DoubleBuilder appendNulls(int count) {
doAppendNulls(count);
return this;
}

@Override
public DoubleBuilder append(Object o) {
if (o == null) {
return appendNulls(1);
}

protected void appendAt(int at, Object o) {
double value;
if (NumericConverter.isFloatLike(o)) {
value = NumericConverter.coerceToDouble(o);
Expand All @@ -124,10 +114,7 @@ public DoubleBuilder append(Object o) {
throw new ValueTypeMismatchException(getType(), o);
}

ensureSpaceToAppend();
setValid(currentSize);
data.put(currentSize++, value);
return this;
data.put(at, value);
}

@Override
Expand All @@ -136,9 +123,8 @@ public void appendBulkStorage(ColumnStorage<?> storage) {
if (storage instanceof DoubleStorage doubleStorage) {
int n = (int) doubleStorage.getSize();
ensureFreeSpaceFor(n);
data.put(currentSize, doubleStorage.getData(), 0, n);
data.put(currentSize(), doubleStorage.getData(), 0, n);
appendValidityMap(doubleStorage.getValidityMap(), n);
currentSize += n;
} else {
var doubleStorage = floatType.asTypedStorage(storage);
long n = doubleStorage.getSize();
Expand Down Expand Up @@ -195,9 +181,7 @@ public void appendBulkStorage(ColumnStorage<?> storage) {
*/
@Override
public DoubleBuilder appendDouble(double value) {
ensureSpaceToAppend();
setValid(currentSize);
data.put(currentSize++, value);
append(value);
Copy link
Member Author

@JaroslavTulach JaroslavTulach Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • this is the place where I started to doubt I am doing the right refactoring
  • DoubleBuilder cannot access currentSize directly
    • as the goal is to encapsulate all the currentSize manipulation in ValidityBuffer
  • so the only thing it can do is to delegate to ValidityBuffer.append and wait for a appendAt callback
  • but that all renders all these appendDouble, appendLong useless...

return this;
}

Expand Down Expand Up @@ -226,7 +210,7 @@ public ColumnStorage<Double> seal() {
*/
final DoubleStorage seal(ColumnStorage<?> otherStorage, StorageType<Double> type) {
ensureFreeSpaceFor(0);
var buf = data.asReadOnlyBuffer().position(0).limit(currentSize);
var buf = data.asReadOnlyBuffer().position(0).limit(currentSize());
var validity = this.validityMap();
return new DoubleStorage(buf, validity, otherStorage);
}
Expand Down Expand Up @@ -306,4 +290,8 @@ final void reportBigDecimalPrecisionLoss(BigDecimal number, double approximation
}
}
}

private int currentSize() {
return Math.toIntExact(getCurrentSize());
}
}
31 changes: 12 additions & 19 deletions std-bits/table/src/main/java/org/enso/table/data/column/builder/InferredDoubleBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
import java.math.BigInteger;
import java.util.BitSet;
import org.enso.base.polyglot.NumericConverter;
import org.enso.table.data.column.storage.ColumnStorage;
import org.enso.table.data.column.storage.type.BigDecimalType;
import org.enso.table.data.column.storage.type.StorageType;
import org.enso.table.data.table.Column;
import org.enso.table.error.ValueTypeMismatchException;
import org.enso.table.problems.ProblemAggregator;
import org.graalvm.polyglot.Context;
Expand Down Expand Up @@ -65,7 +65,7 @@ static InferredDoubleBuilder retypeFromLongBuilder(
@Override
public void copyDataTo(Object[] items) {
int rawN = rawData == null ? 0 : rawData.length;
for (int i = 0; i < currentSize; i++) {
for (int i = 0; i < currentSize(); i++) {
if (!isValid(i)) {
items[i] = null;
} else {
Expand All @@ -85,7 +85,7 @@ public void copyDataTo(Object[] items) {
}

@Override
public void appendBulkStorage(ColumnStorage<?> storage) {
public void appendBulkStorage(Column column) {
throw new UnsupportedOperationException(
"appendBulkStorage is not supported on InferredDoubleBuilder. A DoubleBuilder or"
+ " MixedBuilder should be used instead. This is a bug in the Table library.");
Expand All @@ -96,38 +96,27 @@ public InferredDoubleBuilder appendLong(long integer) {
double convertedFloatValue = (double) integer;
boolean isLossy = integer != (long) convertedFloatValue;
if (isLossy) {
setRaw(currentSize, integer);
setRaw(currentSize(), integer);
precisionLossAggregator.reportIntegerPrecisionLoss(integer, convertedFloatValue);
} else {
isLongCompactedAsDouble.set(currentSize, true);
isLongCompactedAsDouble.set(currentSize(), true);
}
appendDouble(convertedFloatValue);
return this;
}

@Override
public InferredDoubleBuilder append(Object o) {
if (o == null) {
return appendNulls(1);
}

protected void appendAt(int at, Object o) {
if (NumericConverter.isFloatLike(o)) {
appendDouble(NumericConverter.coerceToDouble(o));
} else if (NumericConverter.isCoercibleToLong(o)) {
appendLong(NumericConverter.coerceToLong(o));
} else if (o instanceof BigInteger bigInteger) {
setRaw(currentSize, bigInteger);
setRaw(at, bigInteger);
appendDouble(convertBigIntegerToDouble(bigInteger));
} else {
throw new ValueTypeMismatchException(getType(), o);
}
return this;
}

@Override
public InferredDoubleBuilder appendNulls(int count) {
super.appendNulls(count);
return this;
}

private void setRaw(int ix, Number o) {
Expand Down Expand Up @@ -159,7 +148,7 @@ public boolean canRetypeTo(StorageType<?> type) {
public Builder retypeTo(StorageType<?> type) {
if (type instanceof BigDecimalType) {
Builder res = Builder.getForBigDecimal(getDataSize());
for (int i = 0; i < currentSize; i++) {
for (int i = 0; i < currentSize(); i++) {
if (!isValid(i)) {
res.appendNulls(1);
} else {
Expand All @@ -172,4 +161,8 @@ public Builder retypeTo(StorageType<?> type) {
throw new UnsupportedOperationException();
}
}

private int currentSize() {
return Math.toIntExact(getCurrentSize());
}
}
Loading
Loading