Skip to content

MAPRDB-397, MAPRDB-459#1

Closed
dshylov wants to merge 12 commits into
masterfrom
MAPRDB-397
Closed

MAPRDB-397, MAPRDB-459#1
dshylov wants to merge 12 commits into
masterfrom
MAPRDB-397

Conversation

@dshylov
Copy link
Copy Markdown
Contributor

@dshylov dshylov commented Jan 29, 2018

No description provided.

dshylov and others added 11 commits January 18, 2018 18:11
… language support. Added abstract classes as init commit for ticket.
… language support.

Added set of functions for different data types
… language support.

Removed functions for driver. Rename DriverManager to ConnectionManager.
… language support.

Added part of Value interface and create O*types* files
… language support.

MAPRDB-459 Implement ValueImpl class and subclasses like OTime, ODate, etc..

Added part of ValueImpl, OInterval
…, etc..

Added unit test for OInterval, minor changes in other classes and sample code.
…, etc..

Added OTime and ODate implementation
…, etc..

Added tests for OTime, ODate, OTimestamp. Added new implemented types to ValueImpl.
…for ValueType enum. Added part of Json . Added abstract classes.
Copy link
Copy Markdown
Contributor

@adityakishore adityakishore left a comment

Choose a reason for hiding this comment

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

  • Separate the interfaces + public API classes and implementation+tests into individual pull requests.
  • Move all the future interfaces (DocumentReader, etc) into a separate pull request.
  • Squash multiple commits in a single pull request as one commit.

See comments inline.

Comment thread .idea/vcs.xml
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Accidentally was added

Comment thread Sample.py
store = DocumentStore(connection.get_store(store_name="/test_name"))

# Json string or json map
json_string = "json string"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use a valid JSON Document String, something like

{"name": "Joe", "age": 50, "address": {"street": "555 Moon Way", "city": "Gotham"}}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment thread Sample.py

"""Create a connection, get store, find document by id"""
# create a connection by path:user@password
connection = Connection(ConnectionManager.get_connection(url="ojai:mapr:user@password"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am going to update the functional spec with the new URL format, please update the example with a matching format.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You mean that its must looks like
connection = Connection(ConnectionManager.get_connection(url="hostname:port")) and user@password is not required for connection?

__metaclass__ = ABCMeta

@abstractmethod
def set_id(self, _id):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this single method accept both binary and string types as "_id" value?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, here will check which _id type was given

raise NotImplementedError("Should have implemented this")

@abstractmethod
def get_id(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this return Value type? Is there a way to annotate the return type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,72 @@
class JsonOptions:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class is part of the public API and should be separated from implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

raise NotImplementedError("Should have implemented this")

@abstractmethod
def check_url(self, url):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not needed in the abstract class.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed check_url from abstract class

Comment thread entity/utils/Documents.py
@@ -0,0 +1,13 @@

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not required for initial release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

Comment thread entity/values/Value.py
raise NotImplementedError("Should have implemented this")

@abstractmethod
def as_reader(self):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not required for initial release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

@@ -0,0 +1,5 @@
from abc import ABCMeta, abstractmethod
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not required for initial release.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure

@dshylov dshylov closed this Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants