Navigation Menu

Skip to content
This repository has been archived by the owner on Mar 7, 2018. It is now read-only.

Update the GraphQL queries with new Cassandra schema #87

Merged
merged 28 commits into from Aug 4, 2017

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Jul 31, 2017

Just to get us started, still some things to implement (e.g. extra data needs to get wired from the frontend).

@c-w c-w requested a review from erikschlegel July 31, 2017 15:18
@c-w c-w force-pushed the implement-tiles-resolver branch from 94bbd52 to 8ea73a2 Compare August 1, 2017 16:23
c-w added 4 commits August 1, 2017 09:24
By adding non-queryable columns to the events table that contain all of
an events places and topics, we can re-build a full event just from the
data that we get back from that table. This makes a number of queries
easier and more performant, including the MessagesSchema.event query.
Copy link
Contributor

@Smarker Smarker left a comment

Choose a reason for hiding this comment

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

LGTM

];

cassandraConnector.executeQuery(query, params)
.then(rows => {
const keywords = makeSet(rows.map(row => row.topics), topic => topic);
const keywords = makeSet(rows, row => row.topic);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than const keywords maybe it would be clearer to use const topics so there's one term for topic rather than a topic's name either being keyword or topic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 97a935a.

Copy link
Collaborator

@erikschlegel erikschlegel left a comment

Choose a reason for hiding this comment

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

Looks great. Sorry for the delay. Please opine to the comments below.

AND periodtype = ?
AND pipelinekey = ?
AND externalsourceid = ?
AND conjunctiontopics = ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

we've switched conjunctivetopic1, conjunctivetopic2, conjunctivetopic3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 4b4d59b.

AND externalsourceid = ?
AND event_time <= ?
AND event_time >= ?
`.trim();
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should probably use LIMIT here, as this result set can be large. We only care about the events that display on the news feed which are pages of 15 events.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in ab36b4c.

SELECT *
FROM fortis.events
WHERE pipelinekey = ?
AND eventid IN ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

apparently the limit of items you can include in a C* IN clause is 65535. We should be fine as long as we cap the number of filtered eventid based on a LIMIT threshold from ^^^.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 6bdb30d.

const eventsParams = [
toPipelineKey(args.sourceFilter),
eventIds,
`%${args.fulltextTerm}%`
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

AND periodtype = ?
AND pipelinekey = ?
AND externalsourceid = ?
AND conjunctiontopic1 = ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was a change for CatalystCode/project-fortis-pipeline@f869c20, but conjunctiontopic1 doesn't seem so clear to me just with the 1 appended. I see that it is derived from the first parameter mainEdge in toConjunctionTopics, so maybe there could be a better name for this? But then doing so conjunctiontopic2 and conjunctiontopic3 would have to be changed as well to reflect a change in name of conjunctiontopic1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one for @erikschlegel. I don't particularly care about the naming here because we have quite a few things in the database that require documentation already.

@c-w c-w merged commit 88315dc into master Aug 4, 2017
@c-w c-w deleted the implement-tiles-resolver branch August 4, 2017 15:56
@c-w c-w removed the in progress label Aug 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants