Skip to content
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

Jump to bottom

Metadata Storage extension for Microsoft SqlServer (sqlserver-metadata-storage) #3421

Merged
merged 1 commit into from
Nov 8, 2016
Merged

Metadata Storage extension for Microsoft SqlServer (sqlserver-metadata-storage) #3421

merged 1 commit into from
Nov 8, 2016
+689 −6

Conversation

mark1900
Copy link
Contributor

@mark1900 mark1900 commented Sep 2, 2016 •
edited
Loading

Currently I use this custom extension by adding the required libraries to my Druid classpath (druid/lib/).

  • sqlserver-metadata-storage-*.jar - Druid SqlServer Metadata Storage extension
  • sqljdbc4.jar - The Microsoft JDBC library

Based on:

@fjy
Copy link
Contributor

fjy commented Sep 2, 2016

Thanks @mark1900 !

@drcrallen
Copy link
Contributor

awesome!

@drcrallen drcrallen added this to the 0.9.3 milestone Sep 2, 2016
@drcrallen
Copy link
Contributor

@mark1900 these all need appropriate copyright headers. Check out the other files in druid to see the correct POM and JAVA headers

drcrallen
drcrallen reviewed Sep 2, 2016
View reviewed changes
...r-metadata-storage/src/main/java/io/druid/metadata/storage/sqlserver/SQLServerConnector.java
private static final String PAYLOAD_TYPE = "VARBINARY(MAX)";
private static final String SERIAL_TYPE = "[int] IDENTITY (1, 1)";

public static final int DEFAULT_STREAMING_RESULT_SIZE = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anywhere in MsSql you can point to that verifies this setting? such a thing would be nice in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add code comments in relation to this. On a closer look, I just noticed that I should have used a [bigint] instead of a [int] for the SERIAL_TYPE.

@mark1900 mark1900 mentioned this pull request Sep 2, 2016
Closed
@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from 832120a to 94e2d4b Compare September 2, 2016 20:30
@drcrallen
Copy link
Contributor

@mark1900 this needs docs in https://github.com/druid-io/druid/tree/master/docs/content/development/extensions-contrib . Make sure to mention #3421 (comment) since this PR (unlike the other metadata extensions) does not package the driver's jar by default.

@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from 94e2d4b to 7495ac1 Compare September 2, 2016 22:33
@mark1900
Copy link
Contributor Author

mark1900 commented Sep 2, 2016

@drcrallen Added documentation (sqlserver.md)

@drcrallen
Copy link
Contributor

Cool, @mark1900 can you please fill out the CLA at http://druid.io/community/cla.html ?

👍 after that

drcrallen
drcrallen reviewed Sep 6, 2016
View reviewed changes
docs/content/development/extensions-contrib/sqlserver.md

# Microsoft SQLServer

Make sure to [include](../../operations/including-extensions.html) `sqlserver-metadata-storage` as an extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mentioned in dev-sync today, can you please link to this doc under https://github.com/druid-io/druid/blob/master/docs/content/development/extensions.md in the contrib section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drcrallen Updated extensions.md to link to this new extension.

@drcrallen
Copy link
Contributor

CLA is good

@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from 7495ac1 to 8909385 Compare September 6, 2016 18:26
@drcrallen
Copy link
Contributor

Cool, still 👍

@mark1900
Copy link
Contributor Author

mark1900 commented Sep 6, 2016

Some comments from the JDBI Google Group, on the sqlserver-metadata-storage's use of the JDBI StatementRewriter class.


Using StatementRewriter to rewrite PostgreSQL/MySQL sql syntax to be compatible with Microsoft SqlServer

  • Matthew Hall

    RegExp is a pretty limited language. With so many vendor-specific behaviors and features, I think you'll find RegExp will only get you so far. If this is just a toy project to ease a database migration, then this may work ok for you, but I wouldn't expect it to be a general-purpose solution that works for everybody.

    If you're serious about creating a general purpose tool, I think you'd have more success using EBNF grammar parsers.

  • Steven Schlansker

    You'll also find pretty poor performance with such a rewriter -- the chained replaceAll operations
    will each have to compile the regex repeatedly. You might be saved by caching as long as your statements are cacheable -- but if you use something that generates unique SQL statements (such as @bindin or some custom code) you might shoot yourself in the foot this way.

    I think most people simply maintain parallel sets of SQL per database dialect, unfortunately.

    None of this is to say it won't work. Just be aware that you're going to run into serious limitations
    and this approach probably won't scale well.

@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from 8909385 to e9c9a04 Compare September 6, 2016 19:19
@mark1900
Copy link
Contributor Author

mark1900 commented Sep 6, 2016

In response to the previous comments.

While the current sql transformation might be affected by future sql changes, since Druid's use of SQL is pretty straight forward, the current approach should be sufficient for now.

Also, for better performance I have updated the SQLServerConnector to reuse the sql rewriting patterns.

drcrallen
drcrallen reviewed Sep 6, 2016
View reviewed changes
...r-metadata-storage/src/main/java/io/druid/metadata/storage/sqlserver/SQLServerConnector.java
private static final Pattern REWRITE_PATTERN6 = Pattern.compile("(?i)TRUE");
private static final Pattern REWRITE_PATTERN7 = Pattern.compile("(?i)FALSE");

private ColonPrefixNamedParamStatementRewriter colonPrefixNamedParamStatementRewriter =
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting in this file got all weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drcrallen Updated

@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from e9c9a04 to cc9fcba Compare September 6, 2016 20:25
@mark1900 mark1900 force-pushed the feature-sqlserver-metadata-storage branch from cc9fcba to bc8dfef Compare September 6, 2016 20:32
@fjy
Copy link
Contributor

fjy commented Nov 8, 2016

👍

@fjy fjy merged commit 575aeb8 into apache:master Nov 8, 2016
fundead pushed a commit to fundead/druid that referenced this pull request Dec 7, 2016
@mark1900
Metadata Storage extension for Microsoft SqlServer (sqlserver-metadat…
8e1917b 
…a-storage) (apache#3421)
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
@mark1900 @dgolitsyn
Metadata Storage extension for Microsoft SqlServer (sqlserver-metadat…
b835a2d 
…a-storage) (apache#3421)
@gianm gianm mentioned this pull request Feb 28, 2017
Closed
seoeun25 pushed a commit to seoeun25/incubator-druid that referenced this pull request Feb 25, 2022
@navis
apache#3421 Add more validation applying bloom filter
65bf792
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature Release Notes
Projects
None yet
Milestone
0.10.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mark1900 @fjy @drcrallen @xvrl @gianm