diff --git a/.gitignore b/.gitignore index b9775dea046..26c241c9cae 100644 --- a/.gitignore +++ b/.gitignore @@ -51,3 +51,6 @@ http-client.env.json /doctest/sql-cli/ /doctest/opensearch-job-scheduler/ .factorypath + +# Claude Code files +.claude/ \ No newline at end of file diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/JoinTimeoutHintIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinTimeoutHintIT.java new file mode 100644 index 00000000000..ff7ea88fd1b --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/JoinTimeoutHintIT.java @@ -0,0 +1,102 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.legacy; + +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG; +import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_PEOPLE; + +import java.io.IOException; +import java.util.Locale; +import org.json.JSONArray; +import org.json.JSONObject; +import org.junit.Test; + +/** Integration tests for JOIN_TIME_OUT hint functionality. */ +public class JoinTimeoutHintIT extends SQLIntegTestCase { + + @Override + protected void init() throws Exception { + loadIndex(Index.DOG); + loadIndex(Index.PEOPLE); + loadIndex(Index.GAME_OF_THRONES); + } + + /** + * Core integration test: Verify that JOIN_TIME_OUT hint prevents PIT expiration errors by setting + * PIT keepalive to match the hint value instead of default 60 seconds. + * + *

This is the primary test for the GitHub issue fix. + */ + @Test + public void testJoinTimeoutHintPreventsPointInTimeExpiration() throws IOException { + String query = + String.format( + Locale.ROOT, + "SELECT /*! JOIN_TIME_OUT(120) */ a.firstname, a.lastname, d.dog_name " + + "FROM %s a JOIN %s d ON d.holdersName = a.firstname " + + "WHERE a.age > 25 LIMIT 10", + TEST_INDEX_PEOPLE, + TEST_INDEX_DOG); + + // The main test: this should execute without throwing "Point In Time id doesn't exist" error + JSONObject result = executeQuerySafely(query); + assertNotNull( + "Query with JOIN_TIME_OUT hint should execute without PIT expiration error", result); + + int resultsCount = getResultsCount(result); + assertTrue("Should execute successfully", resultsCount >= 0); + assertTrue("Should respect LIMIT", resultsCount <= 10); + } + + /** + * Regression test: Verify queries without JOIN_TIME_OUT hint still work to ensure implementation + * doesn't break existing functionality + */ + @Test + public void testJoinWithoutTimeoutHintStillWorks() throws IOException { + String query = + String.format( + Locale.ROOT, + "SELECT a.firstname, d.dog_name " + + "FROM %s a JOIN %s d ON d.holdersName = a.firstname " + + "WHERE a.age > 25 LIMIT 5", + TEST_INDEX_PEOPLE, + TEST_INDEX_DOG); + + JSONObject result = executeQuerySafely(query); + assertNotNull("Query without JOIN_TIME_OUT hint should still work", result); + + int resultsCount = getResultsCount(result); + assertTrue("Should work without timeout hint", resultsCount >= 0); + assertTrue("Should respect LIMIT", resultsCount <= 5); + } + + private JSONObject executeQuerySafely(String query) throws IOException { + JSONObject result = executeQuery(query); + + if (result.has("error")) { + throw new RuntimeException("Query failed: " + result.getJSONObject("error")); + } + + return result; + } + + private int getResultsCount(JSONObject result) { + // Check for SQL response format first + if (result.has("total")) { + return result.getInt("total"); + } + + // Check for traditional OpenSearch hits format + if (result.has("hits")) { + JSONArray hits = getHits(result); + return hits.length(); + } + + // No results found + return 0; + } +} diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java b/legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java index 0d879aed9e2..3bf212a0543 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImpl.java @@ -7,6 +7,7 @@ import static org.opensearch.sql.common.setting.Settings.Key.SQL_CURSOR_KEEP_ALIVE; +import java.util.Optional; import java.util.concurrent.ExecutionException; import lombok.Getter; import lombok.Setter; @@ -20,6 +21,7 @@ import org.opensearch.action.search.DeletePitRequest; import org.opensearch.action.search.DeletePitResponse; import org.opensearch.common.action.ActionFuture; +import org.opensearch.common.unit.TimeValue; import org.opensearch.sql.legacy.esdomain.LocalClusterState; import org.opensearch.transport.client.Client; @@ -27,6 +29,7 @@ public class PointInTimeHandlerImpl implements PointInTimeHandler { private final Client client; private String[] indices; + private final Optional customKeepAlive; @Getter @Setter private String pitId; private static final Logger LOG = LogManager.getLogger(); @@ -39,6 +42,20 @@ public class PointInTimeHandlerImpl implements PointInTimeHandler { public PointInTimeHandlerImpl(Client client, String[] indices) { this.client = client; this.indices = indices; + this.customKeepAlive = Optional.empty(); + } + + /** + * Constructor with custom keepalive timeout from JOIN_TIME_OUT hint + * + * @param client OpenSearch client + * @param indices list of indices + * @param customKeepAlive Custom keepalive timeout (from JOIN_TIME_OUT hint) + */ + public PointInTimeHandlerImpl(Client client, String[] indices, TimeValue customKeepAlive) { + this.client = client; + this.indices = indices; + this.customKeepAlive = Optional.ofNullable(customKeepAlive); } /** @@ -50,20 +67,23 @@ public PointInTimeHandlerImpl(Client client, String[] indices) { public PointInTimeHandlerImpl(Client client, String pitId) { this.client = client; this.pitId = pitId; + this.customKeepAlive = Optional.empty(); } /** Create PIT for given indices */ @Override public void create() { - CreatePitRequest createPitRequest = - new CreatePitRequest( - LocalClusterState.state().getSettingValue(SQL_CURSOR_KEEP_ALIVE), false, indices); + TimeValue keepAlive = getEffectiveKeepAlive(); + + LOG.info("Creating PIT with keepalive: {} ({}ms)", keepAlive, keepAlive.getMillis()); + + CreatePitRequest createPitRequest = new CreatePitRequest(keepAlive, false, indices); ActionFuture execute = client.execute(CreatePitAction.INSTANCE, createPitRequest); try { CreatePitResponse pitResponse = execute.get(); pitId = pitResponse.getId(); - LOG.info("Created Point In Time {} successfully.", pitId); + LOG.info("Created Point In Time {} with keepalive {} successfully.", pitId, keepAlive); } catch (OpenSearchSecurityException e) { throw e; } catch (InterruptedException | ExecutionException e) { @@ -86,4 +106,25 @@ public void delete() { throw new RuntimeException("Error occurred while deleting PIT.", e); } } + + /** + * Get effective keepalive value by checking custom timeout first, then falling back to default + * + * @return TimeValue for PIT keepalive + */ + private TimeValue getEffectiveKeepAlive() { + if (customKeepAlive.isPresent()) { + LOG.info( + "Using custom PIT keepalive from JOIN_TIME_OUT hint: {} ({}ms)", + customKeepAlive.get(), + customKeepAlive.get().getMillis()); + return customKeepAlive.get(); + } + + // Fallback: use default + TimeValue defaultKeepAlive = LocalClusterState.state().getSettingValue(SQL_CURSOR_KEEP_ALIVE); + LOG.info( + "Using default PIT keepalive: {} ({}ms)", defaultKeepAlive, defaultKeepAlive.getMillis()); + return defaultKeepAlive; + } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/TableInJoinRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/TableInJoinRequestBuilder.java index 0b37497541d..185fe311d3c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/join/TableInJoinRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/join/TableInJoinRequestBuilder.java @@ -7,6 +7,7 @@ import java.util.List; import org.opensearch.action.search.SearchRequestBuilder; +import org.opensearch.common.unit.TimeValue; import org.opensearch.sql.legacy.domain.Field; import org.opensearch.sql.legacy.domain.Select; @@ -17,6 +18,7 @@ public class TableInJoinRequestBuilder { private List returnedFields; private Select originalSelect; private Integer hintLimit; + private TimeValue hintJoinTimeout; public TableInJoinRequestBuilder() {} @@ -59,4 +61,12 @@ public Integer getHintLimit() { public void setHintLimit(Integer hintLimit) { this.hintLimit = hintLimit; } + + public TimeValue getHintJoinTimeout() { + return hintJoinTimeout; + } + + public void setHintJoinTimeout(TimeValue hintJoinTimeout) { + this.hintJoinTimeout = hintJoinTimeout; + } } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/HashJoinQueryPlanRequestBuilder.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/HashJoinQueryPlanRequestBuilder.java index fee2dc01db3..3e882dbce15 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/HashJoinQueryPlanRequestBuilder.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/HashJoinQueryPlanRequestBuilder.java @@ -5,6 +5,9 @@ package org.opensearch.sql.legacy.query.planner; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensearch.common.unit.TimeValue; import org.opensearch.sql.legacy.query.join.HashJoinElasticRequestBuilder; import org.opensearch.sql.legacy.query.planner.core.Config; import org.opensearch.sql.legacy.query.planner.core.QueryParams; @@ -18,6 +21,7 @@ * how it is assembled. */ public class HashJoinQueryPlanRequestBuilder extends HashJoinElasticRequestBuilder { + private static final Logger LOG = LogManager.getLogger(); /** Client connection to OpenSearch cluster */ private final Client client; @@ -49,6 +53,15 @@ public QueryPlanner plan() { getTotalLimit(), getFirstTable().getHintLimit(), getSecondTable().getHintLimit()); config.configureTermsFilterOptimization(isUseTermFiltersOptimization()); + if (config.timeout() != Config.DEFAULT_TIME_OUT) { + TimeValue joinTimeout = TimeValue.timeValueSeconds(config.timeout()); + LOG.info( + "HashJoinQueryPlanRequestBuilder: Using JOIN_TIME_OUT hint: {} seconds", + config.timeout()); + getFirstTable().setHintJoinTimeout(joinTimeout); + getSecondTable().setHintJoinTimeout(joinTimeout); + } + return new QueryPlanner( client, config, diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java index 207f7efa178..639f7187105 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/planner/physical/node/pointInTime/PointInTime.java @@ -1,7 +1,13 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ package org.opensearch.sql.legacy.query.planner.physical.node.pointInTime; import static org.opensearch.sql.opensearch.storage.OpenSearchIndex.METADATA_FIELD_ID; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.opensearch.common.unit.TimeValue; import org.opensearch.search.SearchHit; import org.opensearch.search.builder.PointInTimeBuilder; @@ -13,6 +19,7 @@ /** OpenSearch Search API with Point in time as physical implementation of TableScan */ public class PointInTime extends Paginate { + private static final Logger LOG = LogManager.getLogger(); private String pitId; private PointInTimeHandlerImpl pit; @@ -35,8 +42,20 @@ public void close() { @Override protected void loadFirstBatch() { - // Create PIT and set to request object - pit = new PointInTimeHandlerImpl(client, request.getOriginalSelect().getIndexArr()); + // Check if this table has JOIN_TIME_OUT hint configured + if (request.getHintJoinTimeout() != null) { + TimeValue customTimeout = request.getHintJoinTimeout(); + LOG.info( + "PointInTime: Creating PIT with JOIN_TIME_OUT hint: {} seconds", + customTimeout.getSeconds()); + pit = + new PointInTimeHandlerImpl( + client, request.getOriginalSelect().getIndexArr(), customTimeout); + } else { + LOG.info("PointInTime: Creating PIT with default timeout value: {}"); + pit = new PointInTimeHandlerImpl(client, request.getOriginalSelect().getIndexArr()); + } + pit.create(); pitId = pit.getPitId(); diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImplTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImplTest.java index bb5f22c26c4..f65b65ede6a 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImplTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/pit/PointInTimeHandlerImplTest.java @@ -4,21 +4,18 @@ */ package org.opensearch.sql.legacy.pit; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertThrows; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.*; import java.util.Collections; +import java.util.List; import java.util.concurrent.ExecutionException; import lombok.SneakyThrows; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.opensearch.action.search.CreatePitAction; @@ -130,4 +127,327 @@ public void testDeleteForFailure() { assertEquals("Error occurred while deleting PIT.", thrownException.getMessage()); verify(mockActionFutureDelete).get(); } + + // ============================================================================ + // NEW TESTS FOR JOIN_TIME_OUT HINT FUNCTIONALITY + // ============================================================================ + + @SneakyThrows + @Test + public void testCreateWithCustomTimeoutFromJoinHint() { + // Test custom timeout from JOIN_TIME_OUT hint + TimeValue customTimeout = TimeValue.timeValueSeconds(120); + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, customTimeout); + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + customHandler.create(); + + // Verify that the CreatePitRequest was called with custom timeout + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + + CreatePitRequest capturedRequest = requestCaptor.getValue(); + assertEquals("Custom timeout should be used", customTimeout, capturedRequest.getKeepAlive()); + assertEquals("PIT ID should be set", PIT_ID, customHandler.getPitId()); + } + + @SneakyThrows + @Test + public void testCreateWithDifferentJoinTimeoutValues() { + // Test various JOIN_TIME_OUT hint values + TimeValue[] testTimeouts = { + TimeValue.timeValueSeconds(30), // 30 seconds + TimeValue.timeValueSeconds(60), // 1 minute + TimeValue.timeValueSeconds(120), // 2 minutes + TimeValue.timeValueSeconds(300), // 5 minutes + TimeValue.timeValueSeconds(600), // 10 minutes + TimeValue.timeValueSeconds(1800) // 30 minutes + }; + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + // Create all handlers and call create() on each + for (TimeValue timeout : testTimeouts) { + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, timeout); + customHandler.create(); + } + + // Verify all timeouts were used correctly + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient, times(testTimeouts.length)) + .execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + + List capturedRequests = requestCaptor.getAllValues(); + assertEquals( + "Should have captured " + testTimeouts.length + " requests", + testTimeouts.length, + capturedRequests.size()); + + // Verify each timeout was preserved + for (int i = 0; i < testTimeouts.length; i++) { + CreatePitRequest request = capturedRequests.get(i); + TimeValue expectedTimeout = testTimeouts[i]; + assertEquals( + "Timeout " + expectedTimeout + " should be preserved", + expectedTimeout, + request.getKeepAlive()); + } + } + + @SneakyThrows + @Test + public void testCreateWithNullCustomTimeout() { + // Test that null custom timeout falls back to default behavior + PointInTimeHandlerImpl customHandler = new PointInTimeHandlerImpl(mockClient, indices, null); + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + customHandler.create(); + + // Should still work (using default from settings) + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), any(CreatePitRequest.class)); + assertEquals("PIT ID should be set even with null timeout", PIT_ID, customHandler.getPitId()); + } + + @SneakyThrows + @Test + public void testCreateDefaultVsCustomTimeout() { + // Test default handler + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + pointInTimeHandlerImpl.create(); + + // Capture all calls and get the first one (default handler) + ArgumentCaptor allRequestsCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + + // Test custom handler + TimeValue customTimeout = TimeValue.timeValueSeconds(300); + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, customTimeout); + + customHandler.create(); + + // Now verify that execute was called exactly 2 times and capture all requests + verify(mockClient, times(2)).execute(eq(CreatePitAction.INSTANCE), allRequestsCaptor.capture()); + + // Get both captured requests + List capturedRequests = allRequestsCaptor.getAllValues(); + assertEquals("Should have captured 2 requests", 2, capturedRequests.size()); + + CreatePitRequest defaultRequest = capturedRequests.get(0); // First call (default handler) + CreatePitRequest customRequest = capturedRequests.get(1); // Second call (custom handler) + + // Verify the custom timeout is preserved + assertEquals("Custom timeout should be preserved", customTimeout, customRequest.getKeepAlive()); + + // Verify they are different (if possible) + assertNotEquals( + "Default and custom timeouts should be different", + defaultRequest.getKeepAlive(), + customRequest.getKeepAlive()); + } + + @Test + public void testConstructorWithCustomTimeout() { + TimeValue customTimeout = TimeValue.timeValueSeconds(180); + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, customTimeout); + + assertNotNull("Handler should not be null", customHandler); + assertNull("PIT ID should be null initially", customHandler.getPitId()); + // Note: We can't directly access the custom timeout, but it will be used in create() + } + + @Test + public void testConstructorWithPitIdOnly() { + String existingPitId = "existing_pit_id_12345"; + PointInTimeHandlerImpl pitHandler = new PointInTimeHandlerImpl(mockClient, existingPitId); + + assertNotNull("Handler should not be null", pitHandler); + assertEquals("PIT ID should be set", existingPitId, pitHandler.getPitId()); + } + + @SneakyThrows + @Test + public void testTimeValueConversions() { + // Test different TimeValue creation methods work correctly + TimeValue secondsTimeout = TimeValue.timeValueSeconds(120); // 2 minutes + TimeValue minutesTimeout = TimeValue.timeValueMinutes(3); // 3 minutes = 180 seconds + TimeValue millisTimeout = TimeValue.timeValueMillis(150000); // 150 seconds + + TimeValue[] timeouts = {secondsTimeout, minutesTimeout, millisTimeout}; + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + // Create all handlers and call create() on each + for (TimeValue timeout : timeouts) { + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, timeout); + customHandler.create(); + } + + // Verify all TimeValue conversions work correctly + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient, times(timeouts.length)) + .execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + + List capturedRequests = requestCaptor.getAllValues(); + assertEquals( + "Should have captured " + timeouts.length + " requests", + timeouts.length, + capturedRequests.size()); + + // Verify each TimeValue was preserved regardless of creation method + for (int i = 0; i < timeouts.length; i++) { + CreatePitRequest request = capturedRequests.get(i); + TimeValue expectedTimeout = timeouts[i]; + assertEquals( + "TimeValue " + expectedTimeout + " should be preserved regardless of creation method", + expectedTimeout, + request.getKeepAlive()); + } + } + + @SneakyThrows + @Test + public void testCompleteLifecycleWithCustomTimeout() { + TimeValue customTimeout = TimeValue.timeValueSeconds(240); + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, customTimeout); + + // Initially no PIT ID + assertNull("PIT ID should be null initially", customHandler.getPitId()); + + // Create PIT + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + customHandler.create(); + assertEquals("PIT ID should be set after creation", PIT_ID, customHandler.getPitId()); + + // Delete PIT + when(mockActionFutureDelete.get()).thenReturn(mockDeletePitResponse); + customHandler.delete(); + + // Verify both operations + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), any(CreatePitRequest.class)); + verify(mockClient).execute(eq(DeletePitAction.INSTANCE), any(DeletePitRequest.class)); + + // Verify custom timeout was used + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + assertEquals( + "Custom timeout should be used", customTimeout, requestCaptor.getValue().getKeepAlive()); + } + + @SneakyThrows + @Test + public void testMultipleIndicesWithCustomTimeout() { + String[] multipleIndices = {"index1", "index2", "index3", "test_left", "test_right"}; + TimeValue customTimeout = TimeValue.timeValueSeconds(360); + + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, multipleIndices, customTimeout); + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + customHandler.create(); + + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + + CreatePitRequest request = requestCaptor.getValue(); + assertEquals( + "Custom timeout should be used with multiple indices", + customTimeout, + request.getKeepAlive()); + assertEquals("PIT ID should be set", PIT_ID, customHandler.getPitId()); + } + + @SneakyThrows + @Test + public void testErrorHandlingWithCustomTimeout() { + TimeValue customTimeout = TimeValue.timeValueSeconds(150); + PointInTimeHandlerImpl customHandler = + new PointInTimeHandlerImpl(mockClient, indices, customTimeout); + + // Setup failure scenario + ExecutionException executionException = + new ExecutionException("PIT creation failed with custom timeout", new RuntimeException()); + when(mockActionFuture.get()).thenThrow(executionException); + + RuntimeException thrownException = + assertThrows(RuntimeException.class, () -> customHandler.create()); + + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), any(CreatePitRequest.class)); + assertNotNull("Exception should have a cause", thrownException.getCause()); + assertEquals( + "Error message should be preserved", + "Error occurred while creating PIT.", + thrownException.getMessage()); + } + + @SneakyThrows + @Test + public void testJoinTimeoutHintScenario() { + // Simulate the scenario where JOIN_TIME_OUT(120) hint is used + // This creates a PIT with 120 seconds keepalive instead of default 60 seconds + + TimeValue joinTimeoutHint = TimeValue.timeValueSeconds(120); + PointInTimeHandlerImpl leftTableHandler = + new PointInTimeHandlerImpl(mockClient, new String[] {"test_left"}, joinTimeoutHint); + PointInTimeHandlerImpl rightTableHandler = + new PointInTimeHandlerImpl(mockClient, new String[] {"test_right"}, joinTimeoutHint); + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + // Create PITs for both tables + leftTableHandler.create(); + rightTableHandler.create(); + + // Verify both handlers use the custom timeout + ArgumentCaptor requestCaptor = + ArgumentCaptor.forClass(CreatePitRequest.class); + verify(mockClient, times(2)).execute(eq(CreatePitAction.INSTANCE), requestCaptor.capture()); + + // Get both captured requests + List capturedRequests = requestCaptor.getAllValues(); + assertEquals("Should have captured 2 requests", 2, capturedRequests.size()); + + CreatePitRequest leftTableRequest = capturedRequests.get(0); // First call (left table) + CreatePitRequest rightTableRequest = capturedRequests.get(1); // Second call (right table) + + // Verify both tables use the JOIN_TIME_OUT hint value + assertEquals( + "Left table should use JOIN_TIME_OUT hint value", + joinTimeoutHint, + leftTableRequest.getKeepAlive()); + assertEquals( + "Right table should use JOIN_TIME_OUT hint value", + joinTimeoutHint, + rightTableRequest.getKeepAlive()); + assertEquals("Left table PIT should be created", PIT_ID, leftTableHandler.getPitId()); + assertEquals("Right table PIT should be created", PIT_ID, rightTableHandler.getPitId()); + } + + @SneakyThrows + @Test + public void testDefaultTimeoutWhenNoHint() { + // Test the original behavior when no JOIN_TIME_OUT hint is provided + PointInTimeHandlerImpl defaultHandler = new PointInTimeHandlerImpl(mockClient, indices); + + when(mockActionFuture.get()).thenReturn(mockCreatePitResponse); + + defaultHandler.create(); + + // Should use default timeout from cluster settings + verify(mockClient).execute(eq(CreatePitAction.INSTANCE), any(CreatePitRequest.class)); + assertEquals("PIT should be created with default timeout", PIT_ID, defaultHandler.getPitId()); + } } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/parser/SqlParserTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/parser/SqlParserTest.java index f00abac812a..4ba48c06cfd 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/parser/SqlParserTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/parser/SqlParserTest.java @@ -1487,6 +1487,279 @@ public void multiSelectMinusScrollCheckDefaultsOneDefault() throws SqlParseExcep Assert.assertEquals(1000, params[2]); } + // ============================================================================ + // JOIN_TIME_OUT Hint Parsing Tests + // ============================================================================ + + @Test + public void joinTimeOutHintBasic() throws SqlParseException { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(120) */ " + + "c.name.firstname,c.parents.father , h.name,h.words from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name " + + "where c.name.firstname='Daenerys'", + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertNotNull(hints); + Assert.assertEquals("hints size was not 1", 1, hints.size()); + Hint hint = hints.get(0); + Assert.assertEquals(HintType.JOIN_TIME_OUT, hint.getType()); + Object[] params = hint.getParams(); + Assert.assertNotNull(params); + Assert.assertEquals("params size was not 1", 1, params.length); + Assert.assertEquals(120, params[0]); + } + + @Test + public void joinTimeOutHintWithSpaces() throws SqlParseException { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT( 180 ) */ " + + "c.name.firstname from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name", + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals(1, hints.size()); + Hint hint = hints.get(0); + Assert.assertEquals(HintType.JOIN_TIME_OUT, hint.getType()); + Assert.assertEquals(180, hint.getParams()[0]); + } + + @Test + public void joinTimeOutHintWithOtherHints() throws SqlParseException { + String query = + String.format( + Locale.ROOT, + "select /*! HASH_WITH_TERMS_FILTER*/ " + + "/*! JOIN_TIME_OUT(300) */ " + + "/*! JOIN_TABLES_LIMIT(1000,null) */ " + + "c.name.firstname from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name", + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals(3, hints.size()); + + // Find JOIN_TIME_OUT hint + Hint timeoutHint = null; + for (Hint hint : hints) { + if (hint.getType() == HintType.JOIN_TIME_OUT) { + timeoutHint = hint; + break; + } + } + + Assert.assertNotNull("JOIN_TIME_OUT hint not found", timeoutHint); + Assert.assertEquals(300, timeoutHint.getParams()[0]); + } + + @Test + public void joinTimeOutHintEdgeCases() throws SqlParseException { + // Test various timeout values + int[] testValues = {30, 60, 120, 300, 600, 1800}; + + for (int timeoutValue : testValues) { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(%d) */ " + + "c.name.firstname from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name", + timeoutValue, + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals( + "Should have exactly 1 hint for timeout " + timeoutValue, 1, hints.size()); + Hint hint = hints.get(0); + Assert.assertEquals( + "Hint type should be JOIN_TIME_OUT", HintType.JOIN_TIME_OUT, hint.getType()); + Assert.assertEquals("Timeout value should match", timeoutValue, hint.getParams()[0]); + } + } + + @Test + public void joinTimeOutHintWithDifferentJoinTypes() throws SqlParseException { + String[] joinTypes = {"JOIN", "LEFT JOIN", "RIGHT JOIN", "INNER JOIN"}; + + for (String joinType : joinTypes) { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(150) */ " + + "c.name.firstname from %s/gotCharacters c " + + "%s %s/gotCharacters h " + + "on c.name.lastname = h.name", + TEST_INDEX_GAME_OF_THRONES, + joinType, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals("Should have hint for " + joinType, 1, hints.size()); + Assert.assertEquals( + "Should be JOIN_TIME_OUT hint", HintType.JOIN_TIME_OUT, hints.get(0).getType()); + Assert.assertEquals("Timeout should be 150", 150, hints.get(0).getParams()[0]); + } + } + + // ============================================================================ + // Negative Test Cases + // ============================================================================ + + @Test + public void joinTimeOutHintInvalidFormat() { + // Test that invalid hint formats are ignored gracefully + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT */ " // Missing parentheses and value + + "c.name.firstname from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name", + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + try { + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + // Should not contain JOIN_TIME_OUT hint if format is invalid + for (Hint hint : hints) { + Assert.assertNotEquals( + "Invalid hint should not be parsed", HintType.JOIN_TIME_OUT, hint.getType()); + } + } catch (SqlParseException e) { + // It's also acceptable if parsing fails for invalid format + Assert.assertTrue("Expected SqlParseException for invalid hint format", true); + } + } + + // ============================================================================ + // Integration Tests with Real Query Scenarios + // ============================================================================ + + @Test + public void joinTimeOutHintWithComplexQuery() throws SqlParseException { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(240) */ /*! JOIN_TABLES_LIMIT(5000,2000) */ " + + "a.firstname, a.lastname, a.age, d.holdersName, d.age as dog_age " + + "from %s/account a " + + "LEFT JOIN %s/dog d on d.holdersName = a.firstname " + + "where a.age > 25 AND d.age < 10 " + + "order by a.age desc " + + "limit 100", + TestsConstants.TEST_INDEX_ACCOUNT, + TEST_INDEX_DOG); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals("Should have 2 hints", 2, hints.size()); + + // Verify JOIN_TIME_OUT hint + Hint timeoutHint = null; + Hint limitHint = null; + for (Hint hint : hints) { + if (hint.getType() == HintType.JOIN_TIME_OUT) { + timeoutHint = hint; + } else if (hint.getType() == HintType.JOIN_LIMIT) { + limitHint = hint; + } + } + + Assert.assertNotNull("JOIN_TIME_OUT hint should be present", timeoutHint); + Assert.assertNotNull("JOIN_TABLES_LIMIT hint should be present", limitHint); + Assert.assertEquals("Timeout should be 240", 240, timeoutHint.getParams()[0]); + Assert.assertEquals("First limit should be 5000", 5000, limitHint.getParams()[0]); + Assert.assertEquals("Second limit should be 2000", 2000, limitHint.getParams()[1]); + } + + // ============================================================================ + // Validation Tests + // ============================================================================ + + @Test + public void joinTimeOutHintValidation() throws SqlParseException { + // Test reasonable timeout values + int[] validTimeouts = {30, 60, 120, 300, 600, 1800, 3600}; // 30 seconds to 1 hour + + for (int timeout : validTimeouts) { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(%d) */ " + + "c.name.firstname from %s/gotCharacters c " + + "JOIN %s/gotCharacters h " + + "on c.name.lastname = h.name", + timeout, + TEST_INDEX_GAME_OF_THRONES, + TEST_INDEX_GAME_OF_THRONES); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + List hints = joinSelect.getHints(); + + Assert.assertEquals("Should parse timeout " + timeout, 1, hints.size()); + Assert.assertEquals("Should be correct value", timeout, hints.get(0).getParams()[0]); + } + } + + @Test + public void joinTimeOutHintPreservationThroughParsing() throws SqlParseException { + String query = + String.format( + Locale.ROOT, + "select /*! JOIN_TIME_OUT(180) */ " + + "a.firstname, a.balance, d.name, d.age " + + "from %s/account a " + + "LEFT JOIN %s/dog d on d.holdersName = a.firstname " + + "where a.age BETWEEN 25 AND 45 " + + "order by a.balance desc", + TestsConstants.TEST_INDEX_ACCOUNT, + TEST_INDEX_DOG); + + JoinSelect joinSelect = parser.parseJoinSelect((SQLQueryExpr) queryToExpr(query)); + + // Verify hint is preserved at the top level + List hints = joinSelect.getHints(); + Assert.assertEquals("Should have one hint", 1, hints.size()); + Hint timeoutHint = hints.get(0); + Assert.assertEquals( + "Should be JOIN_TIME_OUT hint", HintType.JOIN_TIME_OUT, timeoutHint.getType()); + Assert.assertEquals("Should preserve timeout value", 180, timeoutHint.getParams()[0]); + + // Verify other query elements are parsed correctly + Assert.assertNotNull("Should have first table", joinSelect.getFirstTable()); + Assert.assertNotNull("Should have second table", joinSelect.getSecondTable()); + Assert.assertNotNull("Should have connected conditions", joinSelect.getConnectedConditions()); + Assert.assertTrue( + "First table should have order by", joinSelect.getFirstTable().isOrderdSelect()); + } + private SQLExpr queryToExpr(String query) { return new ElasticSqlExprParser(query).expr(); } diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/QueryPlannerConfigTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/QueryPlannerConfigTest.java index 81d6d718b95..43f83d59b2f 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/QueryPlannerConfigTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/unittest/planner/QueryPlannerConfigTest.java @@ -11,6 +11,7 @@ import static org.hamcrest.Matchers.both; import static org.hamcrest.Matchers.emptyArray; import static org.hamcrest.Matchers.is; +import static org.junit.Assert.*; import static org.opensearch.sql.legacy.util.MatcherUtils.featureValueOf; import org.hamcrest.Matcher; @@ -191,6 +192,124 @@ public void multipleConfigCombined() { timeOut(Config.DEFAULT_TIME_OUT))); } + // ============================================================================ + // Enhanced JOIN_TIME_OUT Configuration Tests + // ============================================================================ + + @Test + public void testConfigureTimeOutMethodDirectly() { + Config config = new Config(); + + // Test direct method call + config.configureTimeOut(new Object[] {180}); + assertEquals("Timeout should be set via configureTimeOut method", 180, config.timeout()); + + // Test with different values + config.configureTimeOut(new Object[] {300}); + assertEquals("Timeout should be updated", 300, config.timeout()); + + // Test with empty array (should not change timeout) + int previousTimeout = config.timeout(); + config.configureTimeOut(new Object[] {}); + assertEquals("Empty array should not change timeout", previousTimeout, config.timeout()); + } + + @Test + public void testJoinTimeOutHintOverridesDefault() { + // Test without hint + String sqlWithoutHint = + "SELECT d.name FROM employee e JOIN department d ON d.id = e.departmentId"; + Config configWithoutHint = queryPlannerConfig(sqlWithoutHint); + + // Test with hint + String sqlWithHint = + "SELECT /*! JOIN_TIME_OUT(480) */ " + + "d.name FROM employee e JOIN department d ON d.id = e.departmentId"; + Config configWithHint = queryPlannerConfig(sqlWithHint); + + assertEquals( + "Without hint should use default", Config.DEFAULT_TIME_OUT, configWithoutHint.timeout()); + assertEquals("With hint should override default", 480, configWithHint.timeout()); + assertNotEquals( + "Hint should change the timeout value", + configWithoutHint.timeout(), + configWithHint.timeout()); + } + + @Test + public void testJoinTimeOutHintWithVariousValues() { + int[] timeoutValues = {1, 30, 60, 120, 300, 600, 1800, 3600, 7200}; + + for (int timeoutValue : timeoutValues) { + String sql = + String.format( + "SELECT /*! JOIN_TIME_OUT(%d) */ " + + "d.name FROM employee e JOIN department d ON d.id = e.departmentId", + timeoutValue); + + Config config = queryPlannerConfig(sql); + assertEquals( + "Timeout value " + timeoutValue + " should be preserved", timeoutValue, config.timeout()); + } + } + + @Test + public void testJoinTimeOutHintWithOrderByAndLimit() { + String sql = + "SELECT /*! JOIN_TIME_OUT(200) */ " + + "e.name, d.name FROM employee e JOIN department d ON d.id = e.departmentId " + + "WHERE e.salary > 50000 " + + "ORDER BY e.name ASC " + + "LIMIT 50"; + + Config config = queryPlannerConfig(sql); + + assertEquals("Timeout hint should work with complex query", 200, config.timeout()); + assertEquals("Total limit should be set from LIMIT clause", 50, config.totalLimit()); + } + + @Test + public void testConfigTimeoutImmutabilityAfterSetting() { + Config config = new Config(); + + // Set initial timeout + config.configureTimeOut(new Object[] {180}); + assertEquals("Initial timeout should be set", 180, config.timeout()); + + // Create a new config and verify it has default timeout + Config newConfig = new Config(); + assertEquals( + "New config should have default timeout", Config.DEFAULT_TIME_OUT, newConfig.timeout()); + + // Original config should still have its timeout + assertEquals("Original config should retain its timeout", 180, config.timeout()); + } + + @Test + public void testJoinTimeOutHintValidationRanges() { + // Test boundary values + int[] boundaryValues = { + 1, // Minimum practical value + 59, // Just under default + 60, // Default value + 61, // Just over default + 3600, // 1 hour + 86400, // 24 hours + 604800 // 1 week + }; + + for (int timeout : boundaryValues) { + String sql = + String.format( + "SELECT /*! JOIN_TIME_OUT(%d) */ " + + "d.name FROM employee e JOIN department d ON d.id = e.departmentId", + timeout); + + Config config = queryPlannerConfig(sql); + assertEquals("Boundary value " + timeout + " should be accepted", timeout, config.timeout()); + } + } + private Hint parseHint(String hintStr) { try { return HintFactory.getHintFromString(hintStr);