This is an automated email from the ASF dual-hosted git repository.
jackie pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new 922bd8f1df Fixed a pesky error in PagedPinotOutputStream.getPages when
offsets were in a specific position (#14389)
922bd8f1df is described below
commit 922bd8f1df679279b884eafe4e923978e1197156
Author: Gonzalo Ortiz Jaureguizar <[email protected]>
AuthorDate: Tue Nov 5 23:05:01 2024 +0100
Fixed a pesky error in PagedPinotOutputStream.getPages when offsets were in
a specific position (#14389)
---
.../tests/BaseClusterIntegrationTest.java | 2 +-
.../tests/MultiStageEngineIntegrationTest.java | 22 +++++++++++++++++++
.../segment/spi/memory/PagedPinotOutputStream.java | 10 ++++-----
.../spi/memory/PagedPinotOutputStreamTest.java | 25 ++++++++++++++++++++++
4 files changed, 53 insertions(+), 6 deletions(-)
diff --git
a/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java
b/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java
index cf1fce6fb0..c478e7f9ef 100644
---
a/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java
+++
b/pinot-integration-test-base/src/test/java/org/apache/pinot/integration/tests/BaseClusterIntegrationTest.java
@@ -745,7 +745,7 @@ public abstract class BaseClusterIntegrationTest extends
ClusterTest {
/**
* Run equivalent Pinot and H2 query and compare the results.
*/
- protected void testQuery(@Language("sql") String pinotQuery, String h2Query)
+ protected void testQuery(@Language("sql") String pinotQuery,
@Language("sql") String h2Query)
throws Exception {
ClusterIntegrationTestUtils.testQuery(pinotQuery, getBrokerBaseApiUrl(),
getPinotConnection(), h2Query,
getH2Connection(), null, getExtraQueryProperties(),
useMultiStageQueryEngine());
diff --git
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
index 029b061666..22b515eefa 100644
---
a/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
+++
b/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java
@@ -177,6 +177,28 @@ public class MultiStageEngineIntegrationTest extends
BaseClusterIntegrationTestS
super.testGeneratedQueries(true, true);
}
+ // This query was failing in https://github.com/apache/pinot/issues/14375
+ @Test
+ public void testIssue14375()
+ throws Exception {
+ String query = "SELECT \"DivArrDelay\", \"Cancelled\", \"DestAirportID\" "
+ + "FROM mytable "
+ + "WHERE \"OriginStateName\" BETWEEN 'Montana' AND 'South Dakota' "
+ + "AND \"OriginAirportID\" BETWEEN 13127 AND 12945 "
+ + "OR \"DistanceGroup\" = 4 "
+ + "ORDER BY \"Month\", \"LateAircraftDelay\", \"TailNum\" "
+ + "LIMIT 10000";
+ String h2Query = "SELECT `DivArrDelay`, `Cancelled`, `DestAirportID` "
+ + "FROM mytable "
+ + "WHERE `OriginStateName` BETWEEN 'Montana' AND 'South Dakota' "
+ + "AND `OriginAirportID` BETWEEN 13127 AND 12945 "
+ + "OR `DistanceGroup` = 4 "
+ + "ORDER BY `Month`, `LateAircraftDelay`, `TailNum` "
+ + "LIMIT 10000";
+
+ testQuery(query, h2Query);
+ }
+
@Test
public void testQueryOptions()
throws Exception {
diff --git
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
index b6518f984e..1e54c6fa06 100644
---
a/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
+++
b/pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStream.java
@@ -88,7 +88,10 @@ public class PagedPinotOutputStream extends
PinotOutputStream {
public ByteBuffer[] getPages() {
int numPages = _pages.size();
- boolean lastPageIsEmpty = _written == (numPages - 1) * (long) _pageSize;
+ long lastPageStart = (numPages - 1) * (long) _pageSize;
+ assert lastPageStart >= 0 : "lastPageStart=" + lastPageStart;
+ assert lastPageStart <= _written : "lastPageStart=" + lastPageStart + ",
_written=" + _written;
+ boolean lastPageIsEmpty = _written == lastPageStart;
if (lastPageIsEmpty) {
numPages--;
}
@@ -105,10 +108,7 @@ public class PagedPinotOutputStream extends
PinotOutputStream {
}
if (!lastPageIsEmpty) {
- long startOffset = getCurrentOffset();
- seek(_written);
- result[numPages - 1].limit(_offsetInPage);
- seek(startOffset);
+ result[numPages - 1].limit((int) (_written - lastPageStart));
}
return result;
diff --git
a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStreamTest.java
b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStreamTest.java
index f496511f5f..6e6657727e 100644
---
a/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStreamTest.java
+++
b/pinot-segment-spi/src/test/java/org/apache/pinot/segment/spi/memory/PagedPinotOutputStreamTest.java
@@ -311,4 +311,29 @@ public class PagedPinotOutputStreamTest {
assertEquals(pages[1].position(), 0);
assertEquals(pages[1].limit(), _pageSize);
}
+
+ /**
+ * This tests that getPages works as expected when data has been written to
the stream and all the pages are full.
+ *
+ * Originally there was an error that caused the last returned page to be
empty.
+ *
+ * Detected in <a
href="https://github.com/apache/pinot/issues/14375";>#14375</a>
+ * @throws IOException
+ */
+ @Test
+ void testGetPagesFullPages()
+ throws IOException {
+ byte[] bytes = new byte[_pageSize];
+ _pagedPinotOutputStream.write(bytes);
+ _pagedPinotOutputStream.write(bytes);
+
+ ByteBuffer[] pages = _pagedPinotOutputStream.getPages();
+ assertEquals(pages.length, 2);
+
+ assertEquals(pages[0].position(), 0);
+ assertEquals(pages[0].limit(), _pageSize);
+
+ assertEquals(pages[1].position(), 0);
+ assertEquals(pages[1].limit(), _pageSize);
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
