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.

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

No type information is used to map SQL types to JSON types #73

Open
bserdar opened this issue Oct 17, 2014 · 7 comments
Open

No type information is used to map SQL types to JSON types #73

bserdar opened this issue Oct 17, 2014 · 7 comments
Labels

Comments

@bserdar
Copy link
Contributor

bserdar commented Oct 17, 2014

The Json doc output fields are String, regardless of field type in metadata. The type subsystem must be used to convert database values to Json values in queries, and Json values to database values in insert/update statements. Same goes for bindings: metadata type of fields must be used to convert Json values to/from Java values.

@luan-cestari
Copy link
Collaborator

@bserdar Could you give some more specific technical details of the change? Like the some snippets of the change and which places it would be applied, etc.

@derek63 @bserdar I'm going to take this issue but the milestone and label is not set , so it is in the backlog. May I take it anyway. Burak, don't you think it would be easier if you take this issue?

@derek63 derek63 added the bug label Jan 28, 2015
@derek63 derek63 added this to the Ana milestone Jan 28, 2015
@bserdar
Copy link
Contributor Author

bserdar commented Jan 28, 2015

I am not that familiar with the rdbms code. I'll take a look, determine
what needs to be done, then we can decide who can do it and how.

On Wed, Jan 28, 2015 at 10:53 AM, luan-cestari [email protected]
wrote:

@bserdar https://github.com/bserdar Could you give some more specific
technical details of the change? Like the some snippets of the change and
which places it would be applied, etc.

@derek63 https://github.com/derek63 @bserdar
https://github.com/bserdar I'm going to take this issue but the
milestone and label is not set , so it is in the backlog. May I take it
anyway. Burak, don't you think it would be easier if you take this issue?


Reply to this email directly or view it on GitHub
#73 (comment)
.

@bserdar
Copy link
Contributor Author

bserdar commented Jan 29, 2015

I've been looking at the code of DynVar and how it is used. It looks like in RDBMSProcessor.convertProjection(), DynVar keys are used as JSON document field names. However, as far as I can see, DynVar.keys are populated using table column names/aliases (VariableUpdateRowMapper). It looks like this builds a JSON document containing SQL table column names, not field names. It is during this DynVar setup that I believe the mapping between metadata field and SQL columns are lost. Or, am I missing something?

@luan-cestari
Copy link
Collaborator

Weird, I made comment yesterday here, I think I forgot to click on the comment button, sorry for that @bserdar .

I think you described correct, I would just make an addendum which is the use of DynVar. It was using InOut list before, which is basically the projection fields. The change that resulted in the misused of DynVar occurred when I fixed the projections using ' * ', and I forgot that the DynVar doesn't have the documents names, but the DB names instead.

The solution would not be just switching back to InOut list but enhanced it to get the projected paths (even using features like '*') and for this issue (#73) it would probably need some more information from metadata, IMHO. What do you think? Do you want me to take this issue?

@bserdar
Copy link
Contributor Author

bserdar commented Jan 30, 2015

Yes, please, you're more familiar with the code. The basic idea is to use
the metadata to create result documents that conform to the structure
metadata defines. Once you have the mapping between the metadata fields and
your SQL columns, it should be easy to do.

On Fri, Jan 30, 2015 at 11:31 AM, luan-cestari [email protected]
wrote:

Weird, I made comment yesterday here, I think I forgot to click on the
comment button, sorry for that @bserdar https://github.com/bserdar .

I think you described correct, I would just make an addendum which is the
use of DynVar. It was using InOut list before, which is basically the
projection fields. The change that resulted in the misused of DynVar
occurred when I fixed the projections using ' * ', and I forgot that the
DynVar doesn't have the documents names, but the DB names instead.

The solution would not be just switching back to InOut list but enhanced
it to get the projected paths (even using features like '*') and for this
issue (#73
#73) it
would probably need some more information from metadata, IMHO. What do you
think? Do you want me to take this issue?


Reply to this email directly or view it on GitHub
#73 (comment)
.

@luan-cestari
Copy link
Collaborator

@bserdar could you give me some high level ideas of the conversions that I need to do? By now I can think on the method where now DynVar is used to get the names of fields/columns, which would map a sql.Type to JsonNode.

@bserdar
Copy link
Contributor Author

bserdar commented Jan 30, 2015

You need to maintain a mapping between SQL columns and metadata fields.
When you insert a value to the result Json document, you should insert it
using the metadata field name. When you're setting the value, use
metadataField.type().toJson() to obtain the value you should set to the
output.

jsonDoc.modify(fieldName,field.type().toJson(),true)

should do that.

You have to be careful when inserting array elements. The field name should
be of the form

x.y..z

There can be multiple indexes if the array is multi-level. you can choose
to build a json array manually, or use jsonDoc.modify for this purpose.

On Fri, Jan 30, 2015 at 12:43 PM, luan-cestari [email protected]
wrote:

@bserdar https://github.com/bserdar could you give me some high level
ideas of the conversions that I need to do? By now I can think on the
method where now DynVar is used to get the names of fields/columns, which
would map a sql.Type to JsonNode.


Reply to this email directly or view it on GitHub
#73 (comment)
.

@luan-cestari luan-cestari assigned bserdar and unassigned luan-cestari Feb 2, 2015
@jewzaam jewzaam removed this from the Ana milestone Feb 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants