New HsqlDB connector #23061
New HsqlDB connector #23061
Conversation
prrvchr
force-pushed
the
hsqldb
branch
4 times, most recently
from
b50a6ca to
a559036
Compare
prrvchr
force-pushed
the
hsqldb
branch
4 times, most recently
from
e02c7ae to
b7fddc2
Compare
...uncher/src/main/java/io/trino/tests/product/launcher/env/environment/EnvMultinodeHsqldb.java
Outdated
Show resolved
Hide resolved
|
Well I have to admit that I only have 4GB of memory on my laptop and that I am unable to launch the tests locally (it freezes my terminal) and that therefore I am obliged to push in order to obtain a result... |
prrvchr
force-pushed
the
hsqldb
branch
3 times, most recently
from
35a052b to
7051d03
Compare
| case Types.TIMESTAMP: | ||
| TimestampType timestampType = createTimestampType(getTimestampPrecision(typeHandle.requiredColumnSize())); | ||
| return Optional.of(timestampColumnMapping(timestampType)); | ||
| case Types.TIMESTAMP_WITH_TIMEZONE: |
There was a problem hiding this comment.
I would recommend removing timestamp types from the initial PR as most developers introduce a bug during the implementation.
There was a problem hiding this comment.
Ok, TIMESTAMP and TIMESTAMP_WITH_TIMEZONE were removed from this initial PR (ie: see file HsqlDbClient.java)
| private static Properties getConnectionProperties() | ||
| { | ||
| Properties connectionProperties = new Properties(); | ||
| connectionProperties.setProperty("hsqldb.default_table_type", "cached"); |
There was a problem hiding this comment.
Worth leaving a code comment why specifying this property.
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public abstract class BaseHsqlDbConnectorTest |
There was a problem hiding this comment.
Remove a redundant base class. All code should go to TestHsqlDbConnectorTest.
| protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | ||
| { | ||
| return switch (connectorBehavior) { | ||
| case SUPPORTS_ADD_COLUMN, |
There was a problem hiding this comment.
Please sort alphabetically. Also, remove redundant conditions holding the same value in the base class.
| @Language("RegExp") | ||
| protected String errorMessageForInsertNegativeDate(String date) | ||
| { | ||
| System.out.println("*********************************************************************************************"); |
There was a problem hiding this comment.
Remove redundant System.out.println. Same for others.
| } | ||
| } | ||
|
|
||
| public static void main(String[] args) |
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| public class TestHsqlDbJdbcConfig |
There was a problem hiding this comment.
https://trino.io/docs/current/develop/tests.html
Test classes should be defined as package-private and final.
Same for others.
| public class TestHsqlDbJdbcConfig | ||
| { | ||
| @Test | ||
| public void testIsUrlValid() |
There was a problem hiding this comment.
https://trino.io/docs/current/develop/tests.html
Test methods should be defined as package-private.
Same for others.
|
|
||
| import static io.trino.plugin.hsqldb.TestingHsqlDbServer.DEFAULT_VERSION; | ||
|
|
||
| public class TestHsqlDbLatestConnectorSmokeTest |
There was a problem hiding this comment.
No need to add a smoke test unless you run with the different HSQLDB version from TestHsqlDbConnectorTest.
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
mosabua
added
stale-ignore
|
Sorry for the automated closing @prrvchr - from what I can tell you are waiting for another review and/or merge. Maybe @ebyhr can clarify if other reviewers or other help is needed and potentially post on #core-dev in trino slack. For now I added the stale-ignore label since we want to get this connector in. |
ebyhr
removed
the
stale-ignore
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
Description
Add new HsqlDB connector
Additional context and related issues
I would like to add some pure JDBC functions to Trino in order to be able to provide better integration with LibreOffice Base.
This new connector is a good way to achieve this...
The ultimate goal is to offer LibreOffice Base the possibility via Trino to consolidate or even edit data coming from different DBMS in a very easy way.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: