fix(plugin-singlestore): Varchar type mapping #25476

ScrapCodes · 2025-07-02T10:54:28Z

Description

Currently varchar(len) is mapped to either tinytext, text, mediumtext or longtext of singlestore types. However now singlestore supports varchar with length upto varchar(21844). So, we do not convert varchar length < 21844 into tiny text or text.
More details: https://docs.singlestore.com/cloud/reference/sql-reference/data-types/string-types/

Motivation and Context

We can preserve original table's varchar length as long as they are within the limit of 21844.

Impact

Improved compatibility with singlestore with more accurate type mapping.

Test Plan

Added and fixed existing tests.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

Singlestore connector Changes
* Improved string type mapping, now supports varchar(len) where len <= 21844

steveburnett · 2025-07-02T13:28:49Z

Consider adding documentation to https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/singlestore.rst similar to the Type Mapping topic in the Iceberg Connector doc.

...lestore/src/test/java/com/facebook/presto/plugin/singlestore/TestSingleStoreTypeMapping.java

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java

steveburnett

Thanks for the doc! A formatting nit, and a question.

presto-docs/src/main/sphinx/connector/singlestore.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build. Thanks!

BryanCutler

LGTM, just a question about the commented test case

...lestore/src/test/java/com/facebook/presto/plugin/singlestore/TestSingleStoreTypeMapping.java

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good.

Thanks for the doc!

pratyakshsharma

Couple of minor comments, thank you for the PR.

pratyakshsharma · 2025-07-16T13:39:19Z

presto-docs/src/main/sphinx/connector/singlestore.rst

+  * - ``CHAR(len)``
+    - ``CHAR(len)``
+  * - ``MEDIUMTEXT``
+    - ``VARCHAR(len) 21845 < len < 5592405``


What if the length is exactly 21845? Similarly for 5592045?

Thanks @pratyakshsharma, this needs to be fixed.

I'm still a little confused about the numbers above? Shouldn't it be VARCHAR(len) 21845 <= len < 5592405?

pratyakshsharma · 2025-07-16T13:43:22Z

.../src/test/java/com/facebook/presto/plugin/singlestore/TestSingleStoreDistributedQueries.java

                .row("orderkey", "bigint", "", "")
                .row("custkey", "bigint", "", "")
-                .row("orderstatus", "varchar(85)", "", "")//utf8
+                .row("orderstatus", "varchar(1)", "", "")//utf8


why are we changing these lengths?

These are the actual lengths of those columns, previously it was taking a default length for all varchars lower than 85 represented by tiny text.

pratyakshsharma · 2025-07-16T13:58:25Z

Can we add a test case to cover the scenario of max length supported for varchar/longtext, if possible?

ScrapCodes · 2025-07-20T09:03:50Z

Can we add a test case to cover the scenario of max length supported for varchar/longtext, if possible?

Does presto support that much length for a varchar as of today?

steveburnett

Thanks for the doc! One suggestion of restructuring the doc for you to consider. Let me know what you think!

presto-docs/src/main/sphinx/connector/singlestore.rst

steveburnett

Thanks for the doc! Just a nit of formatting.

presto-docs/src/main/sphinx/connector/singlestore.rst

steveburnett · 2025-10-30T15:39:47Z

Thanks for the release note entry! Minor suggestions:

== RELEASE NOTES ==

Singlestore Connector Changes
* Improved string type mapping, now supports ``varchar(len)`` where ``len <= 21844``.

ScrapCodes · 2025-10-31T05:42:09Z

@steveburnett and @pratyakshsharma can you give me a final approval

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build. Thanks!

BryanCutler

LGTM

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build.

pratyakshsharma

Thank you for the fix and clarifying all my doubts.

pratyakshsharma · 2025-11-13T13:42:31Z

Please check the failures and squash the commits, thanks.

Currently varchar(len) is mapped to either tinytext, text, mediumtext or longtext of singlestore types. However now singlestore supports varchar with length upto varchar(21844). So we need not convert it into tiny text or text.

ScrapCodes requested a review from a team as a code owner July 2, 2025 10:54

prestodb-ci added the from:IBM PR from IBM label Jul 2, 2025

prestodb-ci requested review from a team, BryanCutler and pramodsatya and removed request for a team July 2, 2025 10:54

ScrapCodes requested review from aaneja and pratyakshsharma July 7, 2025 06:32

aaneja reviewed Jul 7, 2025

View reviewed changes

...lestore/src/test/java/com/facebook/presto/plugin/singlestore/TestSingleStoreTypeMapping.java Outdated Show resolved Hide resolved

aaneja reviewed Jul 7, 2025

View reviewed changes

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java Show resolved Hide resolved

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from 135b6bc to c5368ae Compare July 7, 2025 11:09

ScrapCodes requested review from elharo and steveburnett as code owners July 7, 2025 11:09

steveburnett requested changes Jul 7, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/singlestore.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/singlestore.rst Outdated Show resolved Hide resolved

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from c5368ae to 5f6913e Compare July 8, 2025 09:34

steveburnett previously approved these changes Jul 8, 2025

View reviewed changes

BryanCutler reviewed Jul 8, 2025

View reviewed changes

...lestore/src/test/java/com/facebook/presto/plugin/singlestore/TestSingleStoreTypeMapping.java Outdated Show resolved Hide resolved

aaneja reviewed Jul 15, 2025

View reviewed changes

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java Outdated Show resolved Hide resolved

aaneja reviewed Jul 15, 2025

View reviewed changes

presto-singlestore/src/main/java/com/facebook/presto/plugin/singlestore/SingleStoreClient.java Show resolved Hide resolved

ScrapCodes dismissed steveburnett’s stale review via 9760f89 July 15, 2025 11:49

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from 5f6913e to 9760f89 Compare July 15, 2025 11:49

ScrapCodes requested review from BryanCutler, aaneja and steveburnett July 15, 2025 11:56

steveburnett previously approved these changes Jul 15, 2025

View reviewed changes

pratyakshsharma requested changes Jul 16, 2025

View reviewed changes

ScrapCodes dismissed steveburnett’s stale review via b36e9af July 20, 2025 09:47

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from b36e9af to a0c8d0d Compare September 1, 2025 08:53

ScrapCodes requested review from pratyakshsharma and steveburnett September 4, 2025 09:57

steveburnett requested changes Sep 4, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/singlestore.rst Outdated Show resolved Hide resolved

steveburnett requested changes Oct 30, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/singlestore.rst Outdated Show resolved Hide resolved

ScrapCodes changed the title ~~Fixed singlestore varchar type mapping.~~ fix: singlestore varchar type mapping. Oct 30, 2025

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from 669c84d to b5c79e2 Compare October 30, 2025 17:00

ScrapCodes changed the title ~~fix: singlestore varchar type mapping.~~ fix: singlestore varchar type mapping Oct 31, 2025

ScrapCodes changed the title ~~fix: singlestore varchar type mapping~~ Fix: singlestore varchar type mapping Oct 31, 2025

ScrapCodes changed the title ~~Fix: singlestore varchar type mapping~~ fix: singlestore varchar type mapping Oct 31, 2025

ScrapCodes changed the title ~~fix: singlestore varchar type mapping~~ fix: Singlestore varchar type mapping Oct 31, 2025

steveburnett previously approved these changes Oct 31, 2025

View reviewed changes

ScrapCodes dismissed steveburnett’s stale review via e76223f November 3, 2025 07:40

BryanCutler approved these changes Nov 3, 2025

View reviewed changes

steveburnett approved these changes Nov 3, 2025

View reviewed changes

pratyakshsharma approved these changes Nov 13, 2025

View reviewed changes

Fixed singlestore varchar type mapping.

bc3c1e6

Currently varchar(len) is mapped to either tinytext, text, mediumtext or longtext of singlestore types. However now singlestore supports varchar with length upto varchar(21844). So we need not convert it into tiny text or text.

ScrapCodes force-pushed the singlestore_type_mapping_fix branch from e76223f to bc3c1e6 Compare November 20, 2025 16:39

tdcmeehan changed the title ~~fix: Singlestore varchar type mapping~~ fix(plugin-singlestore): Varchar type mapping Nov 21, 2025

tdcmeehan approved these changes Nov 21, 2025

View reviewed changes

ScrapCodes merged commit 6240b16 into prestodb:master Nov 22, 2025
115 of 118 checks passed

ScrapCodes deleted the singlestore_type_mapping_fix branch November 22, 2025 11:28

fix(plugin-singlestore): Varchar type mapping #25476

fix(plugin-singlestore): Varchar type mapping #25476

Conversation

ScrapCodes commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma left a comment

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScrapCodes Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

BryanCutler Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

ScrapCodes Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma commented Jul 16, 2025

Uh oh!

ScrapCodes commented Jul 20, 2025

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett commented Oct 30, 2025

Uh oh!

ScrapCodes commented Oct 31, 2025

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

BryanCutler left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

pratyakshsharma left a comment

Choose a reason for hiding this comment

Uh oh!

ScrapCodes commented Jul 2, 2025 •

edited

Loading

pratyakshsharma Jul 16, 2025 •

edited

Loading